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
1 change: 1 addition & 0 deletions docs/user-guide/rules/list.md
Expand Up @@ -228,6 +228,7 @@ 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.

Expand Down
38 changes: 38 additions & 0 deletions lib/rules/comment-pattern/README.md
@@ -0,0 +1,38 @@
# custom-property-pattern
omril321 marked this conversation as resolved.
Show resolved Hide resolved

Specify a pattern for comments.

<!-- prettier-ignore -->
```css
/* comment */
/** ↑
* The pattern of this */
```

## Options

`regex|string`

A string will be translated into a RegExp like so `new RegExp(yourString)` — so be sure to escape properly.

Given the string:

```
"foo .+"
```

The following patterns are considered violations:

<!-- prettier-ignore -->
```css
/*not starting with foo*/
a { color: red; }
```

The following patterns are _not_ considered violations:

<!-- prettier-ignore -->
```css
/*foo at the beginning*/
a { color: red; }
```
113 changes: 113 additions & 0 deletions lib/rules/comment-pattern/__tests__/index.js
@@ -0,0 +1,113 @@
'use strict';

const { messages, ruleName } = require('..');

testRule({
ruleName,
config: [/foo-.+/],
hudochenkov marked this conversation as resolved.
Show resolved Hide resolved

accept: [
{
code: '/* foo-valid yay */',
},
{
code: `/* foo--
multi-line
\n
\r
\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 {}',
},
],

reject: [
{
code: '/* not foo- */',
message: messages.expected(/foo-.+/),
},
{
code: '/**/',
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)?

},
],
});

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 👍

ruleName,
config: [true],

accept: [
{
code: '/* some comment */',
},
{
code: '/**/',
},
{
code: '.without-comment {}',
},
],
});

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

accept: [
{
code: 'a {} // foo-ok',
},
{
code: '// foo-ok',
},
],

reject: [
{
code: 'a {} // not-foo',
description: 'checks inline scss comments',
message: messages.expected(/foo-.+/),
},
],
});
48 changes: 48 additions & 0 deletions lib/rules/comment-pattern/index.js
@@ -0,0 +1,48 @@
// @ts-nocheck

'use strict';

const _ = require('lodash');
const report = require('../../utils/report');
const ruleMessages = require('../../utils/ruleMessages');
const validateOptions = require('../../utils/validateOptions');

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

});

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

if (!validOptions) {
return;
}

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

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

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

report({
message: messages.expected(normalizedPattern),
node: comment,
result,
ruleName,
});
});
};
}

rule.ruleName = ruleName;
rule.messages = messages;
module.exports = rule;
1 change: 1 addition & 0 deletions lib/rules/index.js
Expand Up @@ -61,6 +61,7 @@ const rules = {
'color-no-invalid-hex': importLazy(() => require('./color-no-invalid-hex'))(),
'comment-empty-line-before': importLazy(() => require('./comment-empty-line-before'))(),
'comment-no-empty': importLazy(() => require('./comment-no-empty'))(),
'comment-pattern': importLazy(() => require('./comment-pattern'))(),
'comment-whitespace-inside': importLazy(() => require('./comment-whitespace-inside'))(),
'comment-word-blacklist': importLazy(() => require('./comment-word-blacklist'))(),
'comment-word-disallowed-list': importLazy(() => require('./comment-word-disallowed-list'))(),
Expand Down