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 autofix to font-family-name-quotes #5806

Merged
merged 22 commits into from Feb 4, 2022

Conversation

MrBrN197
Copy link
Contributor

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

Closes #5805

Is there anything in the PR that needs further explanation?

  • On this line I am using the result object to check against the string-quotes rule to determine whether to replace with single or double-quotes. I'm not sure whether this is wise or not. I'm not sure what order the rules run in so if the font-family-name-quotes rule runs after the string-quotes this might cause it to replace with the wrong type of quotes and string-quotes will not get a chance to fix this.
  • checkFamilyName returns an object when context.fix is enabled and complain doesn't run. modifying the decl.value inside checkFamilyName would result in the other fontFamilyNode.sourceIndexs' being wrong so I defer this until after the loop. I'm not sure if this is the best way to go about this. but to avoid rewriting a lot of the code this is what I decided to do. some clarification on this would be nice.

- add test for rule option `always-unless-required`
- add tests for `always-where-recommended` config option
- add test for `always-where-required` config option
@ybiquitous
Copy link
Member

@MrBrN197 Thank you for creating the pull request and sorry for the late response.

  • On this line I am using the result object to check against the string-quotes rule to determine whether to replace with single or double-quotes. I'm not sure whether this is wise or not. I'm not sure what order the rules run in so if the font-family-name-quotes rule runs after the string-quotes this might cause it to replace with the wrong type of quotes and string-quotes will not get a chance to fix this.

With the current implementation, string-quotes is executed after font-family-name-quotes, that is, by the order defined lib/rules/index.js. See below:

const rules = config.rules
? Object.keys(config.rules).sort(
(a, b) => Object.keys(rulesOrder).indexOf(a) - Object.keys(rulesOrder).indexOf(b),
)
: [];

const rules = {

This behavior is undocumented as far as I know and may be changed in the future. However, since it will be as-is for a while, I recommend removing the code referring config.rules['string-quotes'].

So, it doesn't seem to be a big problem, even if font-family-name-quotes always fixes CSS with single quotes (or double quotes).

  • checkFamilyName returns an object when context.fix is enabled and complain doesn't run. modifying the decl.value inside checkFamilyName would result in the other fontFamilyNode.sourceIndexs' being wrong so I defer this until after the loop. I'm not sure if this is the best way to go about this. but to avoid rewriting a lot of the code this is what I decided to do. some clarification on this would be nice.

OK, I'll review the code more.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

[suggest] Can you please add the following description to the font-family-name-quotes doc?

The [`fix` option](../../../docs/user-guide/usage/options.md#fix) can automatically fix most of the problems reported by this rule.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

[suggest] Can you move fixed: next to code: like other cases?

lib/rules/font-family-name-quotes/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/font-family-name-quotes/__tests__/index.js Outdated Show resolved Hide resolved
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@MrBrN197 I've left some suggestions. I would appreciate them if you could consider them. 😃

lib/rules/font-family-name-quotes/index.js Outdated Show resolved Hide resolved
lib/rules/font-family-name-quotes/index.js Outdated Show resolved Hide resolved
@jeddy3
Copy link
Member

jeddy3 commented Jan 6, 2022

@MrBrN197 Thanks for the pull request.

So, it doesn't seem to be a big problem, even if font-family-name-quotes always fixes CSS with single quotes (or double quotes).

Yes, let's always fix with double quotes. It's what the standard config uses because double quotes are most commonly used in the specification examples.

@jeddy3 jeddy3 changed the title add autofix for font-family-name-quotes rule Add autofix to font-family-name-quotes Jan 6, 2022
@jeddy3
Copy link
Member

jeddy3 commented Jan 17, 2022

@MrBrN197 Thanks for making those changes.

Can you also implement @ybiquitous' two suggestions above, and then I think we'll be ready for a final review?

@MrBrN197
Copy link
Contributor Author

@jeddy3 Hey, Thanks, Yes I'm getting on the other changes.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@MrBrN197 Thank you and sorry for the late response. Great work. 👍🏼

I've left some easy refactoring suggestions, so please consider them.

lib/rules/font-family-name-quotes/index.js Outdated Show resolved Hide resolved
lib/rules/font-family-name-quotes/index.js Show resolved Hide resolved
lib/rules/font-family-name-quotes/index.js Outdated Show resolved Hide resolved
lib/rules/font-family-name-quotes/index.js Outdated Show resolved Hide resolved
lib/rules/font-family-name-quotes/index.js Outdated Show resolved Hide resolved
lib/rules/font-family-name-quotes/index.js Outdated Show resolved Hide resolved
MrBrN197 and others added 5 commits February 4, 2022 00:02
Replace `() => void` with `function`

Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
add return type

Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@MrBrN197
Copy link
Contributor Author

MrBrN197 commented Feb 3, 2022

@ybiquitous Thanks for the feedback. I've made those changes.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@MrBrN197 Great! LGTM 👍🏼

@ota-meshi
Copy link
Member

I think we need to mark the rule as (Autofixable) in the list of rules.
https://github.com/stylelint/stylelint/blob/main/docs/user-guide/rules/list.md

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

@MrBrN197 Thank you! LGTM.

@jeddy3 jeddy3 merged commit 82bd404 into stylelint:main Feb 4, 2022
@jeddy3
Copy link
Member

jeddy3 commented Feb 4, 2022

Changelog entry:

  • Added: font-family-name-quotes autofix (#5806).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support autofix for font-family-name-quotes rule
4 participants