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.

Sign up for GitHub

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

Jump to bottom

Add "comment-pattern" rule #4962

Merged
merged 10 commits into from Oct 15, 2020
Merged

Add "comment-pattern" rule #4962

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4abfda5
added implementation + tests
Oct 2, 2020
13bfb87
Added/updated readme
Oct 4, 2020
7428b0f
applied prettier
Oct 5, 2020
c426c6c
PR fixes (supporting string as option + tests, formatting, improved r…
Oct 5, 2020
2d15bac
specifying the original pattern in the error message + fixing tests
Oct 6, 2020
7802de3
Update lib/rules/comment-pattern/index.js
omril321 Oct 6, 2020
9e847bc
removed irrelevant testRule
Oct 6, 2020
53366ad
Update lib/rules/comment-pattern/README.md
omril321 Oct 7, 2020
b02ec9e
removed redundant tests which are covered in basic checks
Oct 11, 2020
a28b506
Merge branch 'master' of https://github.com/omril321/stylelint
Oct 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/user-guide/rules/list.md
View file
Open in desktop
Expand Up @@ -228,9 +228,9 @@ Grouped first by the following categories and then by the [_thing_](http://apps.

### Comment

- [`comment-pattern`](../../../lib/rules/comment-pattern/README.md): Specify a pattern for comments.
- [`comment-word-blacklist`](../../../lib/rules/comment-word-blacklist/README.md): Specify a list of disallowed words within comments. **(deprecated)**
- [`comment-word-disallowed-list`](../../../lib/rules/comment-word-disallowed-list/README.md): Specify a list of disallowed words within comments.
- [`comment-pattern`](../../../lib/rules/comment-pattern/README.md): Specify a pattern for comments.

### General / Sheet

Expand Down
15 changes: 7 additions & 8 deletions lib/rules/comment-pattern/README.md
View file
Open in desktop
Expand Up @@ -4,22 +4,21 @@ Specify a pattern for comments.

<!-- prettier-ignore -->
```css
/*comment*/
/* ↑ */ a { --foo-: 1px; }
/** ↑
/* comment */
/** ↑
* The pattern of this */
```

## Options

`regex`
`regex|string`

The pattern to match the comments with.
A string will be translated into a RegExp like so `new RegExp(yourString)` — so be sure to escape properly.

Given the regex:
Given the string:

```js
/foo .+/
```
"foo .+"
```

The following patterns are considered violations:
Expand Down
53 changes: 41 additions & 12 deletions lib/rules/comment-pattern/__tests__/index.js
View file
Open in desktop
Expand Up @@ -18,7 +18,7 @@ testRule({
\r\n
\n\r
\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 :)

code: '.without-comment {}',
Expand All @@ -28,11 +28,46 @@ testRule({
reject: [
{
code: '/* not foo- */',
message: messages.expected,
message: messages.expected(/foo-.+/),
},
{
code: '/**/',
message: messages.expected,
message: messages.expected(/foo-.+/),
},
],
});

testRule({
ruleName,
config: ['foo-.+'],

accept: [
{
code: '/* foo-valid yay */',
},
{
code: `/* foo--
multi-line
\n
\r
\r\n
\n\r
\t
*/`,
},
{
code: '.without-comment {}',
},
],

reject: [
{
code: '/* not foo- */',
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)?

},
],
});
Expand All @@ -51,7 +86,7 @@ testRule({
{
code: '.without-comment {}',
},
]
],
});

testRule({
Expand All @@ -62,23 +97,17 @@ testRule({
accept: [
{
code: 'a {} // foo-ok',
description: 'ignored inline scss comments',
},
{
code: '// foo-ok',
description: 'ignored inline scss comments',
},
{
code: '.without-comment { }',
description: 'ignores scss without comments',
},
],

reject: [
{
code: 'a {} // not-foo',
description: 'checks inline scss comments',
message: messages.expected,
message: messages.expected(/foo-.+/),
},
]
],
});
11 changes: 6 additions & 5 deletions lib/rules/comment-pattern/index.js
View file
Open in desktop
Expand Up @@ -10,30 +10,31 @@ const validateOptions = require('../../utils/validateOptions');
const ruleName = 'comment-pattern';

const messages = ruleMessages(ruleName, {
expected: 'Expected comment to match specified pattern',
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

});

function rule(pattern) {
return (root, result) => {
const validOptions = validateOptions(result, ruleName, {
actual: pattern,
possible: [_.isRegExp],
possible: [_.isRegExp, _.isString],
});

if (!validOptions) {
return;
}

const normalizedPattern = _.isString(pattern) ? new RegExp(pattern) : pattern;

root.walkComments((comment) => {
const text = comment.text;


if (pattern.test(text)) {
if (normalizedPattern.test(text)) {
return;
}

report({
message: messages.expected,
message: messages.expected(normalizedPattern),
node: comment,
result,
ruleName,
Expand Down