Add `messageArgs` to 76 rules #6589

kizu · 2023-01-24T18:30:58Z

Adds missing messageArgs to most rules.

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

Original issue: Add support for functional custom messages #4117
Original PR implementing the messageArgs: Add experimental support for custom message arguments #6312

Is there anything in the PR that needs further explanation?

No, it's self-explanatory.

changeset-bot · 2023-01-24T18:31:01Z

🦋 Changeset detected

Latest commit: 636cb67

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
stylelint	Minor

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

ybiquitous

@kizu Thank you for creating the PR. I've suggested a performance improvement, but LGTM.

ybiquitous · 2023-01-25T12:42:45Z

lib/rules/alpha-value-notation/index.js

@@ -120,6 +120,7 @@ const rule = (primary, secondaryOptions, context) => {

 				report({
 					message: messages.expected(unfixed, fixed),


[suggest] Performance tip: we can avoid extra evaluations by passing a function instead of a string (as a result of a function call):

Suggested change

message: messages.expected(unfixed, fixed),

message: messages.expected,

The same is true for the other rules.

@ybiquitous Done!

(sidenote: for some reason, I cannot re-request the review from both of you via github UI — it keeps rotating them; first time seeing this behavior — I wonder if this is the intended behavior or a UI bug?)

jeddy3

@kizu Thank you for your pull request.

Returning to this after focusing on the 15.0.0 release.

Let's revert the changes to any of the rules we deprecated in 15.0.0. It only appears to be a handful of them.

Otherwise, LGTM after making @ybiquitous' suggested changes. Great to see this feature being rolled out across the rules.

jeddy3 · 2023-02-10T18:12:31Z

.changeset/message-args.md

+"stylelint": minor
+---
+
+Added: message arguments to most of the rules that pass them to the built-in messages


Suggested change

Added: message arguments to most of the rules that pass them to the built-in messages

Added: `messageArgs` to <number of> rules

Let's be explicit once we know the number of rules the pull request touches.

@jeddy3 Done! Reverted the changes from the deprecated rules, and updated the changeset with the explicit count (77), also added the mention of the message option to all the READMEs, as it was reported by the “custom message option” test, did it consistently with the rules that already had it — a paragraph present before the ## Options header.

…changes

ybiquitous

@kizu Thank you. LGTM 👍🏼

jeddy3

@kizu Thanks for making the changes.

It's almost there. Before we add these args to our public API will want to ensure they:

are consistent
adhere to our latest conventions

Similar to how we approached making some of our reference data public.

I think I've identified 3 inconsistent rules within their respective groups, e.g. *-list, *-notation and so on.

We can either:

make the required changes to bring them inline in this pull request
remove them from this pull request and tackle them in follow-up ones

Each will require a slight change to rule code and changes to all the testRule cases.

jeddy3 · 2023-02-12T21:28:15Z

lib/rules/selector-pseudo-element-colon-notation/index.js

@@ -75,7 +75,8 @@ const rule = (primary, _secondaryOptions, context) => {
 					}

 					report({
-						message: messages.expected(primary),
+						message: messages.expected,
+						messageArgs: [primary],


Suggested change

messageArgs: [primary],

messageArgs: [unfixed, fixed],

To be aligned with our conventions this should be an "Expected x to be y" message.

Did revert this one — I think changing the message completely is out of scope for this PR, so it is better to handle this one in a follow-up PR.

jeddy3 · 2023-02-12T21:54:06Z

lib/rules/at-rule-property-required-list/index.js

@@ -55,7 +55,8 @@ const rule = (primary) => {
 				}

 				report({
-					message: messages.expected(propertyName, atRuleName),
+					message: messages.expected,
+					messageArgs: [propertyName, atRuleName],


Suggested change

messageArgs: [propertyName, atRuleName],

messageArgs: [atRuleName, propertyName],

This is the opposite way around compared to the other *-list rules where the order of the args matches the order in the name, i.e.

at-rule-property-required-list ^ ^ (arg 1) (arg 2)

Did apply this and the next changes, as they did not require any code additions, just change in the order.

jeddy3 · 2023-02-12T21:55:00Z

lib/rules/rule-selector-property-disallowed-list/index.js

@@ -56,7 +56,8 @@ const rule = (primary) => {

 				if (matchesStringOrRegExp(prop, disallowedProperties)) {
 					report({
-						message: messages.rejected(prop, ruleNode.selector),
+						message: messages.rejected,
+						messageArgs: [prop, ruleNode.selector],


Suggested change

messageArgs: [prop, ruleNode.selector],

messageArgs: [ruleNode.selector, prop],

Order is opposite to other *-list rules. See the other comment.

… fix order in two other

jeddy3

@kizu Thanks for making the changes and sticking with us.

LGTM, thank you.

Let's create a follow-up issue for the colon notation rule.

ybiquitous

@kizu Thanks a lot!

jeddy3 · 2023-02-13T14:26:26Z

@ybiquitous I notice we can't craft a squashed and rebased commit message when using the merge queue, including suffixing the message with the pull request number. The cleanliness of that approach felt good. Do you know if there's a way to combine merge queue with being able to squash commits and then edit the commit message via the GitHub website?

ybiquitous · 2023-02-13T14:35:37Z

Yeah, I notice the inconvenience. And I'm afraid it's not likely to edit the commit message as I know... 😓

ybiquitous · 2023-02-13T14:36:54Z

We can configure the merge queue here.

jeddy3 · 2023-02-14T11:37:00Z

Yeah, I notice the inconvenience. And I'm afraid it's not likely to edit the commit message as I know...

I've turned off the merge queue for now. We can turn it back on if GitHub adds a feature to craft the squashed commit message when adding the pull request to the merge queue.

jeddy3 · 2023-02-14T11:40:52Z

We can turn it back on if GitHub adds a feature to craft the squashed commit message when adding the pull request to the merge queue.

This may happen soon as it's the most liked feedback item.

ybiquitous · 2023-02-14T23:09:08Z

This may happen soon as it's the most liked feedback item.

Good news!

Add messageArgs to most rules that use them

ce08a6a

Adds a changeset

c807d21

ybiquitous requested changes Jan 25, 2023

View reviewed changes

jeddy3 changed the title ~~Add messageArgs to most rules that use them~~ Add messageArgs to xx rules Feb 10, 2023

jeddy3 requested changes Feb 10, 2023

View reviewed changes

kizu added 3 commits February 11, 2023 23:08

Merge branch 'stylelint:main' into message-args

f66c266

Revert changes to messageArgs from deprecated rules, apply requested …

b940924

…changes

Update changeset for the messageArgs with the explicit count

45b7bd4

kizu requested review from jeddy3 and ybiquitous and removed request for jeddy3 and ybiquitous February 11, 2023 23:14

kizu changed the title ~~Add messageArgs to xx rules~~ Add messageArgs to 77 rules Feb 11, 2023

Merge branch 'main' into message-args

0285153

ybiquitous approved these changes Feb 12, 2023

View reviewed changes

jeddy3 requested changes Feb 12, 2023

View reviewed changes

kizu added 2 commits February 13, 2023 14:04

PR review adjustments: revert selector-pseudo-element-colon-notation,…

b7d36f8

… fix order in two other

Fix the readme and the changeset after the last commit

636cb67

kizu changed the title ~~Add messageArgs to 77 rules~~ Add messageArgs to 76 rules Feb 13, 2023

kizu requested a review from jeddy3 February 13, 2023 13:10

jeddy3 approved these changes Feb 13, 2023

View reviewed changes

ybiquitous approved these changes Feb 13, 2023

View reviewed changes

jeddy3 merged commit bc55fa1 into stylelint:main Feb 14, 2023

kizu deleted the message-args branch February 14, 2023 11:50

ybiquitous mentioned this pull request Feb 22, 2023

Add selector-anb-no-unmatchable rule #6678

Merged

mattxwang mentioned this pull request Sep 20, 2023

Stabilise functional support for message option #6966

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `messageArgs` to 76 rules #6589

Add `messageArgs` to 76 rules #6589

kizu commented Jan 24, 2023

changeset-bot bot commented Jan 24, 2023 •

edited

ybiquitous left a comment

ybiquitous Jan 25, 2023

kizu Feb 11, 2023

jeddy3 left a comment

jeddy3 Feb 10, 2023

kizu Feb 11, 2023

ybiquitous left a comment

jeddy3 left a comment •

edited

jeddy3 Feb 12, 2023

kizu Feb 13, 2023

jeddy3 Feb 12, 2023

kizu Feb 13, 2023

jeddy3 Feb 12, 2023

jeddy3 left a comment

ybiquitous left a comment

jeddy3 commented Feb 13, 2023 •

edited

ybiquitous commented Feb 13, 2023

ybiquitous commented Feb 13, 2023

jeddy3 commented Feb 14, 2023

jeddy3 commented Feb 14, 2023

ybiquitous commented Feb 14, 2023

		@@ -120,6 +120,7 @@ const rule = (primary, secondaryOptions, context) => {

		report({
		message: messages.expected(unfixed, fixed),

	message: messages.expected(unfixed, fixed),
	message: messages.expected,

	Added: message arguments to most of the rules that pass them to the built-in messages
	Added: `messageArgs` to <number of> rules

	messageArgs: [propertyName, atRuleName],
	messageArgs: [atRuleName, propertyName],

	messageArgs: [prop, ruleNode.selector],
	messageArgs: [ruleNode.selector, prop],

Add messageArgs to 76 rules #6589

Add messageArgs to 76 rules #6589

Conversation

kizu commented Jan 24, 2023

changeset-bot bot commented Jan 24, 2023 • edited

🦋 Changeset detected

ybiquitous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeddy3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ybiquitous left a comment

Choose a reason for hiding this comment

jeddy3 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeddy3 left a comment

Choose a reason for hiding this comment

ybiquitous left a comment

Choose a reason for hiding this comment

jeddy3 commented Feb 13, 2023 • edited

ybiquitous commented Feb 13, 2023

ybiquitous commented Feb 13, 2023

jeddy3 commented Feb 14, 2023

jeddy3 commented Feb 14, 2023

ybiquitous commented Feb 14, 2023

Add `messageArgs` to 76 rules #6589

Add `messageArgs` to 76 rules #6589

changeset-bot bot commented Jan 24, 2023 •

edited

jeddy3 left a comment •

edited

jeddy3 commented Feb 13, 2023 •

edited