Fix messages of font-weight-notation
#6350
Conversation
🦋 Changeset detectedLatest commit: e109761 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
- Use the "Expected x to be y" message format as possible - Remove the message for invalid `font-weight` notations (it's possible to use the `stylelint-csstree-validator` package instead)
ybiquitous
force-pushed
the
fix-messages-of-font-weight-notation
branch
from
a56e047
to
ff585bf
Compare
|
|
||
| const ruleName = 'font-weight-notation'; | ||
|
|
||
| const messages = ruleMessages(ruleName, { | ||
| expected: (type) => `Expected ${type} font-weight notation`, | ||
| expectedWithActual: (actual, expected) => `Expected "${actual}" to be "${expected}"`, |
There was a problem hiding this comment.
[note] Add expectedWithActual for backward compatibility.
There was a problem hiding this comment.
You're right to add this.
When we first built the rule, it should have only checked normal and bold rather than relative words too. Now we have an odd mix of the named-where-possible and numeric primary options along with the ignore: ["relative"] secondary option.
It's too late to change it now.
Adding expectedWithActual is the best way to deal with backwards compatibility for things like:
stylelint/lib/rules/font-weight-notation/__tests__/index.js
Lines 175 to 183 in 9abc435
| ['normal', '400'], | ||
| ['bold', '700'], | ||
| ]); | ||
| const NUMERIC_TO_KEYWORD = new Map([ | ||
| const NUMERIC_TO_NAMED = new Map([ |
There was a problem hiding this comment.
[note] Rename for more accuracy because both 400 and normal are keywords.
| if (keyword) { | ||
| weightValueNode.value = keyword; | ||
| } | ||
| assertString(namedValue); |
There was a problem hiding this comment.
[note] Use assertString for less if branches. NUMERIC_TO_NAMED.has() === true ensures the return value of NUMERIC_TO_NAMED.get() is a string.
There was a problem hiding this comment.
Thanks for this!
We should also remove this line from the rule README and list of rule docs:
"Also, when named values are expected, require only valid names."
And this line from the rule README:
"Valid font-weight names are normal, bold, bolder, and lighter."
|
|
||
| const ruleName = 'font-weight-notation'; | ||
|
|
||
| const messages = ruleMessages(ruleName, { | ||
| expected: (type) => `Expected ${type} font-weight notation`, | ||
| expectedWithActual: (actual, expected) => `Expected "${actual}" to be "${expected}"`, |
There was a problem hiding this comment.
You're right to add this.
When we first built the rule, it should have only checked normal and bold rather than relative words too. Now we have an odd mix of the named-where-possible and numeric primary options along with the ignore: ["relative"] secondary option.
It's too late to change it now.
Adding expectedWithActual is the best way to deal with backwards compatibility for things like:
stylelint/lib/rules/font-weight-notation/__tests__/index.js
Lines 175 to 183 in 9abc435
|
@jeddy3 Thanks for the review. I’ve addressed your suggestions. |
Follow-up of #6347 (review)
This PR includes the following changes:
font-weightnotations(it's possible to use the
stylelint-csstree-validatorpackage instead)