Skip to content

Add "inline-comment-spacing" lint rule #17893

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Aug 18, 2017

This would check that we write f(/*argName*/ true) and not f(/* argName */ true).
I'd also like to lint for spacing on statement comments like:

/* prefer this? */
foo();

/*prefer this?*/
foo();

But we don't have a consistent style here -- thoughts?

@billti
Copy link
Member

billti commented Aug 18, 2017

Personally, I find it far more readable with the spaces providing some separation. Is there a general community default/agreement on this (i.e. linter defaults)? Or just personal preference?

@ghost ghost force-pushed the inlineCommentSpacing branch from 8b2a429 to 3d377b7 Compare August 18, 2017 17:24
@ghost
Copy link
Author

ghost commented Aug 18, 2017

We get 5000 lines of errors if I invert the test condition and require spaces, so we've been preferring /*comment*/ within argument lists for a while. It would be possible to write a fixer though if we want a mass change. Note that this rule doesn't effect comments on statements, and is just for the f(/*argName*/ true) case.

});
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra trailing lines?

tslint.json Outdated
@@ -11,6 +11,7 @@
"indent": [true,
"spaces"
],
"inline-comment-spacing": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should have a separate rule for "inline-callExpressionArgument-comment-spacing"? Or maybe name it analogously to "boolean-trivia"? eg: "boolean-trivia-spacing"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about renaming boolean-trivia to literal-argument-comment and inline-comment-spacing to argument-comment-spacing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are scripts/tslint rules accessible outside our repo? Are rules in this directory a sort of test-bed for adding rules to tslint itself?

If these are only used by us, then that works for me. We probably shouldn't rename these if someone else is using the boolean-trivia rule.

Copy link
Author

@ghost ghost Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are our own internal rules. Some of them might be suitable to submit to tslint itself, if you want to, although there may be overlap, e.g., no-type-assertion-whitespace may already be handled by tslint's whitespace rule.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 8, 2017

Do we need this new rule? seems like a lot of work, and limited value in my opinion.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 8, 2018

Do not think we need this one. closing for now, please reopen if you feel otherwise.

@mhegazy mhegazy closed this Jan 8, 2018
@ghost ghost deleted the inlineCommentSpacing branch January 11, 2018 22:37
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants