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 "comment-pattern" rule #4962
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great first pull request!
Few small changes required.
CI is failing: To fix formatting errors run npm run format
. To run all test locally use npm test
, if will ensure all tests are passing and not lint errors.
Thanks for the feedback! I've implemented the fixes as suggested. Please let me know if further changes are required.
I understand now why a string option needs to be supported as well, but I believe that specifying it in the requirements will make it more obvious for first-timers :) |
Indeed, it was missed. I've added it to mentioned message. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for making changes. One small change to make.
message: messages.expected(/foo-.+/), | ||
}, | ||
{ | ||
code: '/**/', | ||
message: messages.expected(/foo-.+/), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should match config, and in config we have string.
message: messages.expected(/foo-.+/), | |
}, | |
{ | |
code: '/**/', | |
message: messages.expected(/foo-.+/), | |
message: messages.expected('foo-.+'), | |
}, | |
{ | |
code: '/**/', | |
message: messages.expected('foo-.+'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hudochenkov got it, fixed now.
I thought that we wanted to show the normalizedPattern
in the message, instead of the pattern that was supplied in the config. I've updated the implementation + tests as suggested :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that we wanted to show the normalizedPattern in the message, instead of the pattern that was supplied in the config.
I didn't thought about that :) Now I'm not sure which one is better :)
@stylelint/contributors what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems reasonable to me to always show the same form pattern "foo"
, not pattern "/foo/"
.
Even in different config file formats, it seems users expect the same output of a message, for example:
# .stylelintrc.yml
rules:
comment-pattern: ["foo"]
// .stylelintrc.js
module.exports = {
rules: {
"comment-pattern": [/foo/] // equal to ["foo"]
}
}
$ bin/stylelint.js a.css --config .stylelintrc.yml
a.css
1:1 ✖ Expected comment to match specified pattern "foo" comment-pattern
$ bin/stylelint.js a.css --config .stylelintrc.js
a.css
1:1 ✖ Expected comment to match specified pattern "/foo/" comment-pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ybiquitous forgive me but I might be confused.. Are you suggesting to strip the slashes from the pattern when generating an error message? If so, how should regexes with modifiers look? e.g. /foo-.+/i
or /foo-.+/ig
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting to strip the slashes from the pattern when generating an error message?
Yes, I thought we could keep the same appearance of an error message in any config formats.
If so, how should regexes with modifiers look? e.g.
/foo-.+/i
or/foo-.+/ig
?
Sorry, I missed regex modifiers. Surely your current implementaion seems better if we need to include the modifiers. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok :)
Are there any further changes or fixes that are required (in this area or anywhere else)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@omril321 Looking good, thank you. Just a couple of minor requests from me.
\t | ||
*/`, | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove these .without-comment {}
tests as they are duplicated in the basic checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I didn't know the basic checks :)
lib/rules/comment-pattern/index.js
Outdated
const ruleName = 'comment-pattern'; | ||
|
||
const messages = ruleMessages(ruleName, { | ||
expected: (pattern) => `Expected comment to match specified pattern "${pattern}"`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expected: (pattern) => `Expected comment to match specified pattern "${pattern}"`, | |
expected: (pattern) => `Expected comment to match pattern "${pattern}"`, |
Let's drop "specified" as we show the pattern in the message. I think including the pattern is a good idea, but let's create a follow-up issue to do this consistently across the other *-pattern
rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I can open an issue soon
], | ||
}); | ||
|
||
testRule({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this testRule
.
I'd expect config: [true]
to produce an invalid option error. This is likely a short coming in the validateOptions
function, that we can pick up another time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this. I'll open an issue soon 👍
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, once the redundant code: '.without-comment {}',
are removed.
Thanks for your patience 🙏 I wasn't thorough enough when I removed redundant tests . I've just removed them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes.
LGTM. Thank you for contributing to stylelint!
|
* master: (46 commits) Update CHANGELOG.md Add ignoreAtRules to property-no-unknown (#4965) Bump eslint from 7.11.0 to 7.12.1 (#5017) Bump typescript from 4.0.3 to 4.0.5 (#5016) Bump lint-staged from 10.4.0 to 10.5.1 (#5014) Bump remark-cli from 8.0.1 to 9.0.0 (#4996) Bump jest-circus from 26.5.3 to 26.6.1 (#5009) Bump got from 11.7.0 to 11.8.0 (#5007) Bump jest from 26.5.3 to 26.6.1 (#5008) Refactor formatter tests (#4988) Fix `isStandardSyntaxDeclaration.test.js` that use callbacks (#4972) Update CHANGELOG.md Add "comment-pattern" rule (#4962) Update CHANGELOG.md Show the pattern in "*-pattern" rule messages (#4975) Enable ESLint `no-shadow` and add disable comments (#4986) Report disables in the same manner as lints (#4973) Update dependencies (#4982) Fix some tests that use callbacks (#4970) Use own vendor utility instead of PostCSS (#4942) (#4963) ...
Closes #4887
No, it's self-explanatory.
I do want to mention that it is my first PR for an open source project. I'll appreciate any input :)