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

Add "comment-pattern" rule #4962

Merged
merged 10 commits into from Oct 15, 2020
Merged

Add "comment-pattern" rule #4962

merged 10 commits into from Oct 15, 2020

Conversation

omril321
Copy link
Contributor

@omril321 omril321 commented Oct 4, 2020

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

Closes #4887

Is there anything in the PR that needs further explanation?

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 :)

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.

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.

docs/user-guide/rules/list.md Outdated Show resolved Hide resolved
lib/rules/comment-pattern/README.md Outdated Show resolved Hide resolved
lib/rules/comment-pattern/README.md Outdated Show resolved Hide resolved
lib/rules/comment-pattern/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/comment-pattern/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/comment-pattern/__tests__/index.js Show resolved Hide resolved
lib/rules/comment-pattern/index.js Outdated Show resolved Hide resolved
lib/rules/comment-pattern/index.js Outdated Show resolved Hide resolved
@omril321
Copy link
Contributor Author

omril321 commented Oct 5, 2020

Thanks for the feedback! I've implemented the fixes as suggested. Please let me know if further changes are required.
By the way, as a feedback of my own, I didn't know that adding a rule which accepts regex as an option requires support for a string option as well. I think I got confused by the original message here

...
Primary option: regex
...

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 :)

@hudochenkov
Copy link
Member

By the way, as a feedback of my own, I didn't know that adding a rule which accepts regex as an option requires support for a string option as well. I think I got confused by the original message here

Indeed, it was missed. I've added it to mentioned message. Thanks!

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.

Thank for making changes. One small change to make.

Comment on lines 66 to 70
message: messages.expected(/foo-.+/),
},
{
code: '/**/',
message: messages.expected(/foo-.+/),
Copy link
Member

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.

Suggested change
message: messages.expected(/foo-.+/),
},
{
code: '/**/',
message: messages.expected(/foo-.+/),
message: messages.expected('foo-.+'),
},
{
code: '/**/',
message: messages.expected('foo-.+'),

Copy link
Contributor Author

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 :)

Copy link
Member

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?

Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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. 👍

Copy link
Contributor Author

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)?

Copy link
Member

@jeddy3 jeddy3 left a 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
*/`,
},
{
Copy link
Member

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.

Copy link
Contributor Author

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 :)

const ruleName = 'comment-pattern';

const messages = ruleMessages(ruleName, {
expected: (pattern) => `Expected comment to match specified pattern "${pattern}"`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

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({
Copy link
Member

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.

Copy link
Contributor Author

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 👍

omril321 and others added 2 commits October 6, 2020 21:33
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

LGTM! 👏

Copy link
Member

@jeddy3 jeddy3 left a 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.

@omril321
Copy link
Contributor Author

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

Copy link
Member

@jeddy3 jeddy3 left a 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!

@hudochenkov hudochenkov merged commit cba295e into stylelint:master Oct 15, 2020
@hudochenkov
Copy link
Member

  • Added: comment-pattern rule (#4962).

m-allanson added a commit that referenced this pull request Nov 5, 2020
* 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)
  ...
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.

Add comment-pattern
4 participants