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

Accept multiple spaces before comment in selector-list-comma-newline-after rule #2915

Merged
merged 3 commits into from Sep 23, 2017

Conversation

3 participants
@pkuczynski
Contributor

pkuczynski commented Sep 21, 2017

Fixes #2912

// If there's a // comment, that means there has to be a newline
// ending the comment so we're fine
if (nextThreeChars === " //") {
if (nextChars.match(/^\s+\/\//)) {
return;
}
// If there is a space and then a comment begins, look for the newline

This comment has been minimized.

@ntwb

ntwb Sep 22, 2017

Member

Can this comment be updated to reflect the new functionality of the code please

How about: If there are spaces and then a comment begins, look for the newline

@@ -2,6 +2,7 @@
- Added: `length-zero-no-unit` autofix ([#2861](https://github.com/stylelint/stylelint/issues/2861)).
- Added: `selector-max-specificity` support for level 4 evaluation context pseudo-classes ([#2857](https://github.com/stylelint/stylelint/issues/2857)).
- Fixed: aAccept multiple spaces before comments in `selector-list-comma-newline-after` rule ([#2912](https://github.com/stylelint/stylelint/issues/2912)).

This comment has been minimized.

@ntwb

ntwb Sep 22, 2017

Member

Typo here Accept, not aAccept ;)

This comment has been minimized.

@ntwb

ntwb Sep 22, 2017

Member

Actually, can you remove this change to CHANGELOG.md entirely from this PR please.

We update CHANGELOG.md per the docs here https://github.com/stylelint/stylelint/blob/master/docs/developer-guide/pull-requests.md after the PR has been merged, it helps us avoid CHANGELOG.md merge conflicts from multiple PRs :)

This comment has been minimized.

@pkuczynski

pkuczynski Sep 22, 2017

Contributor

Sure I can. In rvm project we actually enforce it the other way around asking people to provide CHANGELOG entries :)

This comment has been minimized.

@jeddy3

jeddy3 Sep 22, 2017

Member

We did it that way around originally, but we constantly got merge conflicts in our PRs.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Sep 22, 2017

@pkuczynski Thanks for this, especially the comprehensive tests. If you can make @ntwb changes, then I believe this will be good to go.

@pkuczynski

This comment has been minimized.

Contributor

pkuczynski commented Sep 22, 2017

@jeddy3 done! :)

@jeddy3

jeddy3 approved these changes Sep 22, 2017

LGTM

@pkuczynski

This comment has been minimized.

Contributor

pkuczynski commented Sep 22, 2017

Yeah, us too. But then very easy to solve using gihtub ui :) At least that's how we deal with it. But that does not matter I guess :)

@ntwb

ntwb approved these changes Sep 23, 2017

@jeddy3 jeddy3 merged commit e53c60e into stylelint:master Sep 23, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 95.682%
Details
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Sep 23, 2017

  • Fixed: selector-list-comma-newline-after false positives for shared-line comments separated by more than once space (#2915).
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Oct 5, 2017

@pkuczynski Thanks again for your first-time contribution. Your fix was just released in 8.2.0.

@pkuczynski

This comment has been minimized.

Contributor

pkuczynski commented Oct 5, 2017

Glad to hear that! I will have a look at my other PR tonight...

@pkuczynski pkuczynski deleted the pkuczynski:2912-selector-list-comma-newline-after branch Oct 5, 2017

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