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

Add no-double-slash-css-comments rule #675

Merged
merged 7 commits into from Feb 3, 2016

Conversation

3 participants
@kangax
Contributor

kangax commented Feb 2, 2016

No description provided.

@kangax kangax referenced this pull request Feb 2, 2016

Closed

no-double-slash-css-comments #661

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 2, 2016

Thanks for working on this, @kangax!

Do you think it's comprehensive enough? With a name like no-double-slash-css-comments, I'd expect it to complain about // comments anywhere, not just within declarations and selectors. Do you agree?

If so, it seems that maybe what needs to be done is search through the whole stylesheet string for // (outside of real comments and strings), and complain any time it occurs. If that sounds right, the utility that would be useful to for this kind of thing is styleSearch(), and some rules that use it to search the whole stylesheet string are max-empty-lines, no-eol-whitespace, and max-line-length.

Then we could add tests checking for // anywhere — in media queries, outside of any blocks at the root level, etc.

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 2, 2016

@kangax : And nice job picking up on all the patterns for defining and testing rules!

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Feb 2, 2016

@kangax Thanks for this :)

I'd expect it to complain about // comments anywhere

I believe // outside of the two places tested for will result in a parse error. I think the intent of this rule is to disallow these two types of CSS comment hacks.

Then we could add tests checking for // anywhere — in media queries, outside of any blocks at the root level, etc.

So we won't be able to check these locations with the regular parser. We can add another rule, perhaps called comment-no-double-dash which tests in these locations when using a parser that supports these comment styles.

Does that make sense, or have a misunderstood things? :)

(Marking as "needs revision" for the code style errors)

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 2, 2016

@jeddy3 , @kangax : Oh, sorry that must have been part of the original conversation that I somehow missed! What a bonehead. If we're preventing the patterns in that article, then a couple of things:

  • The article mentions commenting out an @keyframes declaration. Should we be checking at-rules?
  • Let's make sure to provide a link to that article to help explain what the rule is about, and clearly differentiate it from preprocessor // comments.
  • I'm actually in favor of changing the name to include hack or something else that more clearly distinguishes what it's targeting from preprocessor //. I think that almost anybody seeing the rule will assume that it's about processor // comments, and be confused by the results.
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Feb 2, 2016

The article mentions commenting out an @Keyframes declaration. Should we be checking at-rules?

According to astexplorer PostCSS parses at-rules with // in front of them as selectors. So, I think we should add tests... but they should pass without any changes to the code.

Let's make sure to provide a link to that article

Definitely! @kangax Can you make the change please?

I'm actually in favor of changing the name to include hack

@kangax Sorry to flip-flop on this one, but I agree with David. If the rule name confused him that it's very likely to confuse users who aren't familiar with stylelint. Also, using the word "hacks" is used in the companion stylehacks linter (which also relies on odd parser behaviours), and so keeping things consistent would be great.

I think once the following are done, we'll be ready to merge :)

  • Add at-rule tests
  • Add article link to docs
  • Rename to no-double-slash-comment-hacks.
  • Fix code style
@kangax

This comment has been minimized.

Contributor

kangax commented Feb 2, 2016

Hey folks.

So yes, like @jeddy3 mentions, I didn't include // in the other context (such as a {color:pink} // foo) for the reason of PostCSS parser simply erroring out with "unrecognized property" (or something along those lines). So the code wouldn't be parseable in the first place and naturally the error wouldn't sneak in (as could be the case with // in other context). It also can't be tested, of course.

I'll add a test for @ rule now and a link to the doc. Good points!

As far as name goes... I still believe that "hack" is confusing. In my frontend life (of almost 10 years) I've never thought of // in CSS as hacks. Yes, I'm a small sample size. But I also don't recall anyone referring to them this way (aside from good old browser-targeting hacks based on feature support — http://stackoverflow.com/a/20541859/130652; those are the only "hacks" in context of CSS that I know of and that is apparently exactly what stylehacks linter takes care of).

The reason I don't think it will be confused with what's targeting by preprocessor is because we used the word CSS (double-slash-css-comments seems pretty explicit).

If you're still against it, that's cool. I'm not completely hung up on this; but I do think that it's a reasonable name choice :)

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 2, 2016

I get it, yeah ... I would still prefer if we could think of some qualifier that more clearly distinguished this from preprocessor // comments. Could we brainstorm a bit?

The basic goal in my mind is to cause anybody who doesn't immediately know what this is about to check out the docs before making assumptions. So some less-familiar word or phrase in the name would be nice.

How about no-double-slash-next-construct-comments, or no-next-construct-comments?

@davidtheclark

This comment has been minimized.

Huh, looks like the ruleTester() is not properly validating column numbers. I'll file another bug for that.

@kangax

This comment has been minimized.

Contributor

kangax commented Feb 2, 2016

(double negative sucks but...)

  • no-nonstandard-double-slash-comments
  • no-nonstandard-double-slash-css-comments
  • no-unsupported-double-slash-comments
  • no-unsupported-double-slash-css-comments
  • no-double-slash-comment-lookalikes
@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 2, 2016

no-invalid-double-slash-comments (since a validator like the W3C's will reject it)

@kangax

This comment has been minimized.

Contributor

kangax commented Feb 2, 2016

no-invalid-double-slash-comments (since a validator like the W3C's will reject it)

Sounds good to me

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 2, 2016

Ok. I defer to @jeddy3 on final decisions for naming, so let's see what he thinks.

Otherwise, I just have a couple more addition ideas:

  • Please note in the docs that these invalid comments are distinct from preprocessor // single-line comments.
  • Please write a test or two validating that when using the SCSS parser this rule does not complain. For an example of a test with the SCSS parser, have a look at
    const testRuleScss = ruleTester(rule, ruleName, {
  • Finally, please add a note for the curious, something like "If you didn't know you could do this in CSS, read http://www.xanthir.com/b4U10"
@kangax

This comment has been minimized.

Contributor

kangax commented Feb 2, 2016

When you say "preprocessor // single-line comments" what exactly do you mean? Does Sass/SCSS have preprocessor directives via double-slash comments? I don't recall but I guess I'm forgetting something?

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 2, 2016

Does this make sense, to you and as part of the documentation?

If you are using a preprocessor that allows // single-line comments (e.g. Sass, Less, Stylus), this rule will not complain about those. They are compiled into standard CSS comments by your preprocessor, so stylelint will consider them valid. This rule only complains about the lesser-known method of using // to "comment out" a single line of code in regular CSS. (If you didn't know this was possible, have a look at "Single Line Comments (//) in CSS".)

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Feb 2, 2016

no-invalid-double-slash-comments

Sgtm.

As does your paragraph for the docs :-)

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 3, 2016

Excellent!

davidtheclark added a commit that referenced this pull request Feb 3, 2016

Merge pull request #675 from kangax/master
Add no-double-slash-css-comments rule

@davidtheclark davidtheclark merged commit 728f325 into stylelint:master Feb 3, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Feb 4, 2016

Thanks guys! Looks great :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment