Add `function-url-quotes` autofix #6558

mattxwang · 2023-01-05T07:19:04Z

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

Closes #6500.

Is there anything in the PR that needs further explanation?

Two possible code smells I want to call out. In both cases, I opted to pick a "dirty" solution that doesn't change existing behaviour and minimizes/localizes the changed code. However, there are probably more correct/elegant ways to do this. Advice is welcome!

I was able to address these thanks to code review, but I'll leave the comments for posterity.

First, this rule operates on both declarations and at-rules. It seems like in checkArgs, this type information is "erased". To resolve this, I have my utility functions addQuotes and removeQuotes take in a union type, and then use a crude 'params' in node to differentiate the type.

This ... feels wrong? I was a bit unsure if there's a better way for me to do this (either from a TS perspective, or perhaps a Stylelint util that I'm missing).

Secondly, the previous iteration of this rule lowercases declarations and at-rule parameters before passing them in as arguments to functionArgumentsSearch. This ensures that the rule matches UrL and the like.

However, this has the side effect of lowercasing the declaration value and/or at-rule parameters. Then, when I apply the autofix, I propagate this lowercase change (which means that the autofix also inadvertently lowercases the code).

To resolve this problem, I pass in a case-insensitive regex to functionArgumentsSearch. I don't think this is technically wrong, but it is likely a performance regression; I didn't want to change functionArgumentsSearch implementation instead. With that in mind, any suggestions on better ways to do this?

Thanks in advance - implementing this autofix was trickier than I thought!

changeset-bot · 2023-01-05T07:19:07Z

🦋 Changeset detected

Latest commit: 853d2ba

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

lib/rules/function-url-quotes/index.js

lib/rules/function-url-quotes/__tests__/index.js

ybiquitous · 2023-01-10T13:20:09Z

First, this rule operates on both declarations and at-rules. It seems like in checkArgs, this type information is "erased". To resolve this, I have my utility functions addQuotes and removeQuotes take in a union type, and then use a crude 'params' in node to differentiate the type.

Good point. I think it will be more readable to split into 2 functions with a single purpose. For example:

if (context.fix) {
	if (node.type === 'atrule') {
		addQuotesForAtRule(node, trimmedArg, complaintIndex);
	} else {
		addQuotesForDecl(node, trimmedArg, complaintIndex);
	}

	return;
}

The same is true for removeQuotes.

ybiquitous · 2023-01-10T13:26:42Z

To resolve this problem, I pass in a case-insensitive regex to functionArgumentsSearch. I don't think this is technically wrong, but it is likely a performance regression; I didn't want to change functionArgumentsSearch implementation instead. With that in mind, any suggestions on better ways to do this?

I think the solution makes sense and will not degrade performance much. But benchmarking is beneficial. Please consider sharing benchmark results via the npm run benchmark-rule script:

stylelint/docs/developer-guide/rules.md

Line 267 in b4fd2d5

npm run benchmark-rule -- ruleName ruleOptions [ruleContext]

…specific functions

…xable Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>

mattxwang · 2023-01-10T18:38:10Z

Thanks @ybiquitous for the quick review, I appreciate it! I believe I've addressed all the comments.

In addition, thanks for suggesting benchmarks! I ran them (results below), and it seems like there's no sizeable impact on the rule behaviour with or without autofix. I'm unsure if I'm using the commands correctly, small timescale, or variance on my local machine (M1 macOS) represents all other use cases, but it seems reasonable.

Benchmarks

With Autofix

$ npm run benchmark-rule -- function-url-quotes always "{\"fix\": \"true\"}"

> stylelint@14.16.1 benchmark-rule
> node scripts/benchmark-rule.mjs function-url-quotes always {"fix": "true"}

Warnings: 0
Mean: 13.63671329657534 ms
Deviation: 3.1167337864783593 ms

$ npm run benchmark-rule -- function-url-quotes never "{\"fix\": \"true\"}"

> stylelint@14.16.1 benchmark-rule
> node scripts/benchmark-rule.mjs function-url-quotes never {"fix": "true"}

Warnings: 0
Mean: 13.534337736986304 ms
Deviation: 2.8618163492841986 ms

Without Autofix

Note: we should expect no change, since this PR doesn't touch non-autofix behaviour.

HEAD of this PR:

$ npm run benchmark-rule -- function-url-quotes always

> stylelint@14.16.1 benchmark-rule
> node scripts/benchmark-rule.mjs function-url-quotes always

Warnings: 0
Mean: 13.244889730666669 ms
Deviation: 2.8424667783141677 ms

$ npm run benchmark-rule -- function-url-quotes never 

> stylelint@14.16.1 benchmark-rule
> node scripts/benchmark-rule.mjs function-url-quotes never

Warnings: 14
Mean: 13.58528738150685 ms
Deviation: 3.2287915266088905 ms

main right now:

$ npm run benchmark-rule -- function-url-quotes always

> stylelint@14.16.1 benchmark-rule
> node scripts/benchmark-rule.mjs function-url-quotes always

Warnings: 0
Mean: 13.292001378666669 ms
Deviation: 3.100038171534227 ms

$ npm run benchmark-rule -- function-url-quotes never

> stylelint@14.16.1 benchmark-rule
> node scripts/benchmark-rule.mjs function-url-quotes never

Warnings: 14
Mean: 13.420950610810808 ms
Deviation: 3.1724774966395293 ms

ybiquitous

Thank you for addressing the reviews and sharing the performance results. LGTM! 👍🏼

Mouvedia · 2023-01-12T14:53:37Z

@mattxwang does it use the string-quotes rule value—if not set to null—to choose between " and ' for the fix?

If not, that fix will require multiple runs.
i.e. #3853

mattxwang · 2023-01-14T02:44:58Z

@mattxwang does it use the string-quotes rule value—if not set to null—to choose between " and ' for the fix?

If not, that fix will require multiple runs. i.e. #3853

Ah - no, it doesn't. I implemented this similar to the behaviour in the other two quotes rules (font-family-name-quotes, selector-attribute-quotes) which both default to " as the quote. I believe I've added similar behaviour (i.e. in all three autofix cases, multiple runs are needed).

Is there past precedence / a blueprint for a rule inspecting the value of another rule? It may make sense to simply fix all of these at once in one PR (I am happy to contribute this if desired).

Mouvedia · 2023-01-14T14:14:45Z

It may make sense to simply fix all of these at once in one PR (I am happy to contribute this if desired).

If you don't I probably will. Either way, it needs to be discussed in an issue: #6577
Since it's a deprecated rule there are specific challenges to be resolved first before implementation.

draft: failing / poor approach

4611534

mattxwang added 3 commits January 8, 2023 23:11

Refactor to use raws-based utilities

e0624f1

adds changeset

b0d86c7

Improves test coverage; "fixes" case insensitivity and autofix

d1cea88

mattxwang changed the title ~~(heavy draft): Add function-url-quotes autofix~~ Add function-url-quotes autofix Jan 9, 2023

mattxwang commented Jan 9, 2023

View reviewed changes

lib/rules/function-url-quotes/index.js Outdated Show resolved Hide resolved

mattxwang commented Jan 9, 2023

View reviewed changes

lib/rules/function-url-quotes/index.js Outdated Show resolved Hide resolved

mattxwang marked this pull request as ready for review January 9, 2023 07:53

mattxwang mentioned this pull request Jan 9, 2023

Add message arguments to declaration-property-unit-allowed-list #6570

Merged

minor changeset formatting

3b7a7d0

ybiquitous reviewed Jan 10, 2023

View reviewed changes

lib/rules/function-url-quotes/index.js Show resolved Hide resolved

ybiquitous reviewed Jan 10, 2023

View reviewed changes

lib/rules/function-url-quotes/__tests__/index.js Outdated Show resolved Hide resolved

mattxwang and others added 4 commits January 10, 2023 10:09

Use declarationValueIndex instead of custom util

511f882

Reduce test duplication by adding fix: true to existing rules

47a9d8a

Split up addQuotes and removeQuotes into at-rule and declaration-…

870e2d5

…specific functions

Adds documentation / metadata to indicate that the rule is now autofi…

a25ca75

…xable Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>

Merge branch 'main' into autofix-function-url-quotes

853d2ba

ybiquitous approved these changes Jan 11, 2023

View reviewed changes

mattxwang merged commit 6c74739 into main Jan 11, 2023

mattxwang deleted the autofix-function-url-quotes branch January 11, 2023 00:34

Mouvedia mentioned this pull request Jan 14, 2023

Use string-quotes for rules that require quote fixes #6577

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `function-url-quotes` autofix #6558

Add `function-url-quotes` autofix #6558

mattxwang commented Jan 5, 2023 •

edited

changeset-bot bot commented Jan 5, 2023 •

edited

ybiquitous commented Jan 10, 2023 •

edited

ybiquitous commented Jan 10, 2023

mattxwang commented Jan 10, 2023

ybiquitous left a comment

Mouvedia commented Jan 12, 2023 •

edited

mattxwang commented Jan 14, 2023

Mouvedia commented Jan 14, 2023 •

edited

Add function-url-quotes autofix #6558

Add function-url-quotes autofix #6558

Conversation

mattxwang commented Jan 5, 2023 • edited

changeset-bot bot commented Jan 5, 2023 • edited

🦋 Changeset detected

ybiquitous commented Jan 10, 2023 • edited

ybiquitous commented Jan 10, 2023

mattxwang commented Jan 10, 2023

Benchmarks

With Autofix

Without Autofix

ybiquitous left a comment

Choose a reason for hiding this comment

Mouvedia commented Jan 12, 2023 • edited

mattxwang commented Jan 14, 2023

Mouvedia commented Jan 14, 2023 • edited

Add `function-url-quotes` autofix #6558

Add `function-url-quotes` autofix #6558

mattxwang commented Jan 5, 2023 •

edited

changeset-bot bot commented Jan 5, 2023 •

edited

ybiquitous commented Jan 10, 2023 •

edited

Mouvedia commented Jan 12, 2023 •

edited

Mouvedia commented Jan 14, 2023 •

edited