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 function-url-quotes autofix #6500

Closed
jeddy3 opened this issue Dec 1, 2022 · 0 comments · Fixed by #6558
Closed

Add function-url-quotes autofix #6500

jeddy3 opened this issue Dec 1, 2022 · 0 comments · Fixed by #6558
Labels
status: ready to implement is ready to be worked on by someone type: new autofix a new autofix for an existing rule

Comments

@jeddy3
Copy link
Member

jeddy3 commented Dec 1, 2022

What is the problem you're trying to solve?

The rule doesn't autofix.

What solution would you like to see?

To have it autofix as it's:

I've labelled the issue as ready to implement. Please consider contributing if anyone has time.

There are steps to add autofix to a rule in the Developer guide.

@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: new autofix a new autofix for an existing rule labels Dec 1, 2022
mattxwang added a commit that referenced this issue Jan 11, 2023
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!

Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready to implement is ready to be worked on by someone type: new autofix a new autofix for an existing rule
Development

Successfully merging a pull request may close this issue.

1 participant