Skip to content
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

spaced-comment warns on "don't minify" comment blocks #2777

Closed
ljharb opened this issue Jun 16, 2015 · 14 comments · Fixed by DavidKindler/meteor#3 · May be fixed by iamstoick/javascript#11
Closed

spaced-comment warns on "don't minify" comment blocks #2777

ljharb opened this issue Jun 16, 2015 · 14 comments · Fixed by DavidKindler/meteor#3 · May be fixed by iamstoick/javascript#11
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@ljharb
Copy link
Contributor

ljharb commented Jun 16, 2015

Typically, the first comment block in a file starts with /*! to indicate to a minifier like uglify that it should not strip the comment block (a "prelude").

Could we add an option to the spaced-comment rule that allows this pattern?

@gyandeeps gyandeeps added the triage An ESLint team member will look at this issue soon label Jun 16, 2015
@gyandeeps
Copy link
Member

You can use markers option.
http://eslint.org/docs/rules/spaced-comment

spaced-comment:[2, always, {markers:["!"]}]

@ljharb
Copy link
Contributor Author

ljharb commented Jun 16, 2015

aha, thanks! Sorry I missed that in my reading :-)

However, I tried adding ["!"] as the exceptions list, and I get "Expected exception block, space or tab after /* in comment". The comment block is https://github.com/es-shims/es7-shim/blob/master/es7-shim.js#L1-L5

@gyandeeps
Copy link
Member

after further looking into this, I dont think there is an clean way to get rid of the error.
Should we just ignore multi-line block comments for this rule? it would also solve #2766 .

@ljharb
Copy link
Contributor Author

ljharb commented Jun 16, 2015

Personally I wouldn't want to allow:

/*blah
 *blah
 */

but that might be pretty specific. If this only covered single-line comments I still think it'd be useful.

@gyandeeps
Copy link
Member

To solve your case, you have to do what you just specified above. as of now.

@ljharb
Copy link
Contributor Author

ljharb commented Jun 16, 2015

Then for now I'll have to leave the rule disabled :-( thanks for the quick replies

@gyandeeps
Copy link
Member

Found the problem. As of now if you use a marker then you have to specify a space to tab after it.
But we can start allowing \n then the above scenario would work out.
Here is the code: https://github.com/eslint/eslint/blob/master/lib/rules/spaced-comment.js#L49

@ljharb
Copy link
Contributor Author

ljharb commented Jun 16, 2015

I didn't use a marker though, I created an exception.

At any rate, I'd assume any syntactically valid whitespace would be allowed, so definitely that would include a newline, and possibly other chars (still getting confirmation on that tho)

@ljharb
Copy link
Contributor Author

ljharb commented Jun 16, 2015

Valid whitespace is http://people.mozilla.org/~jorendorff/es6-draft.html#sec-white-space, so I'd think we'd want to include all of those?

@gcochard
Copy link
Contributor

I think this is an edge case introduced when the logic for single line comments was added to multi-line comments. We should definitely allow any whitespace between lines, and we should also split milti-line comments on newline and treat each line individually.

As for this issue, @ljharb do you have any whitespace after the !? I believe the rule looks for exceptions and then EOL, with nothing in between.

@ljharb
Copy link
Contributor Author

ljharb commented Jun 16, 2015

As you can see on https://github.com/es-shims/es7-shim/blob/master/es7-shim.js#L1-L5, no, no trailing whitespace, just EOL.

@feross
Copy link
Contributor

feross commented Jun 17, 2015

I just ran into this issue with this comment block:

/**
 * This is a block comment
 */

Neither "exceptions": ["*"] or "markers": ["*"] does the trick. But removing the newline works:

/** This is a block comment */

Newline should be treated as valid whitespace in this case.

@gyandeeps
Copy link
Member

thats what i proposed above and i agree.

@gyandeeps gyandeeps added bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Jun 17, 2015
@nzakas nzakas added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jun 17, 2015
@gyandeeps
Copy link
Member

@ljharb Once the fix is done you should be able to use markers: ["!"] for your scenario.
@feross your scenario is more like #2766 . So it will be resolved when that issue gets resolved.

nzakas added a commit that referenced this issue Jun 18, 2015
Fix: Allow blocked comments with markers and new-line (fixes #2777)
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 7, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
5 participants