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

Improve inline comment merging #4950

Merged
merged 4 commits into from Sep 24, 2020
Merged

Improve inline comment merging #4950

merged 4 commits into from Sep 24, 2020

Conversation

jathak
Copy link
Contributor

@jathak jathak commented Sep 23, 2020

Which issue, if any, is this issue related to?

Fixes #4937.

Is there anything in the PR that needs further explanation?

This now only starts a merge when the comment begins with stylelint- and either the first line contains -- or the second line starts with --.

The merge will end whenever another line starts with stylelint- to allow for adjacent stylelint commands.

This now only starts a merge when the comment begins with `stylelint-`
and either the first line contains `--` or the second line starts with
`--`.

The merge will end whenever another line starts with `stylelint-` to
allow for adjacent stylelint commands.
if (
!(
isInlineComment(comment) &&
comment.text.startsWith('stylelint-') &&
Copy link
Member

Choose a reason for hiding this comment

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

This will match commits with stylelint-whatever. Users could put stylelint plugins names in comments. I think we should change comments starting with stylelint-disable and stylelint-enable to eliminate problems.

Same applies for a stylelint- check below.

* @param {PostcssComment} comment
*/
function isStylelintCommand(comment) {
return /^stylelint-(disable|enable)/.test(comment.text);
Copy link
Member

Choose a reason for hiding this comment

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

I just remembered that we have disableCommand and enableCommand in this file. I think we should use them instead of hardcoded values. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense. Should I just call startsWith with both or should I put a compiled RegExp in a global variable and use that? (since a regex literal couldn't be used in this case)

Copy link
Member

Choose a reason for hiding this comment

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

I would choose startsWith because it's easier to understand.

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Great job!

I've checked on https://github.com/twbs/bootstrap repository this changes. No more issues!

@hudochenkov hudochenkov merged commit 4f068be into master Sep 24, 2020
@hudochenkov hudochenkov deleted the comment-merging-fix branch September 24, 2020 22:14
@hudochenkov
Copy link
Member

  • Fixed: regression for disable commands and adjacent double-slash comments (#4950).

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

Successfully merging this pull request may close these issues.

Fix regression for disable commands and adjacent double-slash comments
4 participants