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 deprecated rule warnings #6561

Merged
merged 6 commits into from Jan 26, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
5 changes: 5 additions & 0 deletions .changeset/gold-llamas-pump.md
@@ -0,0 +1,5 @@
---
"stylelint": minor
---

Added: deprecated rule warnings
2 changes: 1 addition & 1 deletion lib/__tests__/integration.test.js
Expand Up @@ -60,7 +60,7 @@ describe('integration test expecting warnings', () => {
});

it('number and type', () => {
expect(result.messages).toHaveLength(5);
expect(result.messages).toHaveLength(6);
expect(result.messages.every((m) => m.type === 'warning')).toBeTruthy();
expect(result.messages.every((m) => m.plugin === 'stylelint')).toBeTruthy();
});
Expand Down
4 changes: 2 additions & 2 deletions lib/formatters/__tests__/githubFormatter.test.js
Expand Up @@ -47,13 +47,13 @@ test('githubFormatter', () => {
const returnValue = {
ruleMetadata: {
foo: { url: 'https://stylelint.io/rules/foo' },
bar: { fixable: true },
bar: { url: 'https://stylelint.io/rules/bar', fixable: true, deprecated: true },
},
};

expect(githubFormatter(results, returnValue))
.toBe(`::error file=path/to/file.css,line=1,col=2,endLine=1,endColumn=5,title=Stylelint problem::Unexpected "foo" (foo) - https://stylelint.io/rules/foo
::warning file=a.css,line=10,col=20,title=Stylelint problem::Unexpected "bar" (bar) [maybe fixable]
::warning file=a.css,line=10,col=20,title=Stylelint problem::Unexpected "bar" (bar) [maybe fixable, deprecated] - https://stylelint.io/rules/bar
::error file=a.css,line=20,col=1,title=Stylelint problem::Cannot parse foo (foo-error)
::error file=a.css,line=20,col=3,title=Stylelint problem::Anonymous error`);
});
28 changes: 24 additions & 4 deletions lib/formatters/__tests__/verboseFormatter.test.js
Expand Up @@ -337,14 +337,30 @@ describe('verboseFormatter', () => {
severity: 'error',
text: 'Unexpected foo',
},
{
line: 1,
column: 2,
rule: 'no-bar',
severity: 'error',
text: 'Unexpected bar',
},
{
line: 1,
column: 2,
rule: 'no-baz',
severity: 'error',
text: 'Unexpected baz',
},
],
deprecations: [],
invalidOptionWarnings: [],
},
];
const returnValue = {
ruleMetadata: {
'no-foo': { url: 'https://stylelint.io', fixable: true },
'no-foo': { url: 'https://stylelint.io', fixable: true, deprecated: true },
'no-bar': { url: 'https://stylelint.io', fixable: true },
'no-baz': { url: 'https://stylelint.io', deprecated: true },
},
};

Expand All @@ -353,14 +369,18 @@ describe('verboseFormatter', () => {
expect(output).toBe(stripIndent`
file.css
1:2 × Unexpected foo no-foo
1:2 × Unexpected bar no-bar
1:2 × Unexpected baz no-baz

1 problem (1 error, 0 warnings)
3 problems (3 errors, 0 warnings)

1 source checked
file.css

1 error found
no-foo: 1 (maybe fixable)
3 errors found
no-foo: 1 (maybe fixable, deprecated)
no-bar: 1 (maybe fixable)
no-baz: 1 (deprecated)

You may fix some problems with the "--fix" option.`);
});
Expand Down
12 changes: 10 additions & 2 deletions lib/formatters/githubFormatter.js
Expand Up @@ -35,7 +35,15 @@ function buildMessage(msg, metadata) {
if (!metadata) return msg;

const url = metadata.url ? ` - ${metadata.url}` : '';
const fixable = metadata.fixable ? ' [maybe fixable]' : '';

return `${msg}${fixable}${url}`;
let additional = [
metadata.fixable ? 'maybe fixable' : '',
metadata.deprecated ? 'deprecated' : '',
]
.filter(Boolean)
.join(', ');

additional = additional ? ` [${additional}]` : '';

return `${msg}${additional}${url}`;
}
13 changes: 9 additions & 4 deletions lib/formatters/verboseFormatter.js
Expand Up @@ -72,12 +72,17 @@ module.exports = function verboseFormatter(results, returnValue) {
const metadata = returnValue.ruleMetadata;

for (const [rule, list] of Object.entries(problemsByRule)) {
const meta = metadata[rule];
const fixable = meta && meta.fixable ? ' (maybe fixable)' : '';
const meta = metadata[rule] || {};

output += dim(` ${ruleLink(rule, meta)}: ${list.length}${fixable}\n`);
let additional = [meta.fixable ? 'maybe fixable' : '', meta.deprecated ? 'deprecated' : '']
.filter(Boolean)
.join(', ');

if (!fixableProblemsFound && meta && meta.fixable) {
additional = additional ? ` (${additional})` : '';

output += dim(` ${ruleLink(rule, meta)}: ${list.length}${additional}\n`);

if (!fixableProblemsFound && meta.fixable) {
fixableProblemsFound = true;
}
}
Expand Down
20 changes: 17 additions & 3 deletions lib/lintPostcssResult.js
Expand Up @@ -16,7 +16,7 @@ const getStylelintRule = require('./utils/getStylelintRule');
* @param {StylelintConfig} config
* @returns {Promise<any>}
*/
function lintPostcssResult(stylelintOptions, postcssResult, config) {
module.exports = function lintPostcssResult(stylelintOptions, postcssResult, config) {
postcssResult.stylelint.ruleSeverities = {};
postcssResult.stylelint.customMessages = {};
postcssResult.stylelint.ruleMetadata = {};
Expand Down Expand Up @@ -77,6 +77,10 @@ function lintPostcssResult(stylelintOptions, postcssResult, config) {
continue;
}

if (ruleFunction.meta && ruleFunction.meta.deprecated) {
warnDeprecatedRule(postcssResult, ruleName);
}

const ruleSettings = config.rules && config.rules[ruleName];

if (ruleSettings === null || ruleSettings[0] === null) {
Expand Down Expand Up @@ -114,7 +118,7 @@ function lintPostcssResult(stylelintOptions, postcssResult, config) {
}

return Promise.all(performRules);
}
};

/**
* There are currently some bugs in the autofixer of Stylelint.
Expand All @@ -131,4 +135,14 @@ function isFixCompatible({ stylelint }) {
return true;
}

module.exports = lintPostcssResult;
/**
* @param {PostcssResult} result
* @param {string} ruleName
* @returns {void}
*/
function warnDeprecatedRule(result, ruleName) {
result.warn(
`The "${ruleName}" rule is deprecated and will be removed in the next major release.`,
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
`The "${ruleName}" rule is deprecated and will be removed in the next major release.`,
`The "${ruleName}" rule is deprecated.`,

Let's keep it succinct in the terminal to avoid a wall of text.

Copy link
Contributor

@Mouvedia Mouvedia Jan 12, 2023

Choose a reason for hiding this comment

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

What about having just one sentence with a list of the matched rules instead of multiple sentences?
e.g. The "string-quotes", "unit-case" and "foo-bar" rules are deprecated and will be removed in the next major release.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the advice. I've simplified warning messages via ff83ba8. Here is a screenshot:

image

I keep multiple sentences because each message may have a reference, and it's hard to include reference(s) in one sentence. See the code below:

if (reference) {
line += dim(` See: ${underline(reference)}`);
}

Instead, I've avoided repeated "Deprecation Warning" messages in each line and appended a " - " prefix to each line.

What about this change?

Copy link
Contributor

@Mouvedia Mouvedia Jan 13, 2023

Choose a reason for hiding this comment

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

[suggestion]

Let's take stylelint config standard as the baseline.
Let's say a user resets about 19% of the deprecated rules; that clocks at around 12 lines out of potentially 64.
In that scenario I would prefer a counter and the usage of or etc. for more than 3.
e.g.

`${count} deprecation warnings for rules ${rules[0]},  ${rules[1]}, ${rules[2]}, etc.`

Copy link
Member

Choose a reason for hiding this comment

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

What about this change?

SGTM, thank you.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Mouvedia I understand your concern. But, if reference is specified with a rule deprecation, how should we display it with or etc.? I believe we need to consider also plugin rules, not only the built-in ones.

Copy link
Contributor

@Mouvedia Mouvedia Jan 14, 2023

Choose a reason for hiding this comment

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

Alright, as long as we have an ENV variable—or something similar—that hides those warnings it should be OK.
@ybiquitous Does the --quiet option hide these warnings?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does the --quiet option hide these warnings?

I'm afraid the answer is no because these warnings are not related to a warning severity produced by lint rules. We currently don't have a way to suppress this kind of warning, except for using a custom formatter.

{ stylelintType: 'deprecation' },
);
}
4 changes: 3 additions & 1 deletion scripts/visual-config.json
Expand Up @@ -10,6 +10,8 @@
"severity": "warning"
}
],
"import-notation": "string"
"import-notation": "string",
"unit-case": null,
"string-quotes": "single"
}
}