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.

Sign up for GitHub

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

Jump to bottom

Add declaration-block-no-redundant-longhand-properties autofix #6580

Merged
merged 17 commits into from Feb 10, 2023
Merged

Add declaration-block-no-redundant-longhand-properties autofix #6580

merged 17 commits into from Feb 10, 2023

Conversation

mattxwang
Copy link
Member

@mattxwang mattxwang commented Jan 17, 2023 •
edited

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

Closes #3304.

Is there anything in the PR that needs further explanation?

See my review comment below - I think this probably needs some reviewing!

In particular, this PR involved me writing a lot of manual code/is prone to error.

@changeset-bot
Copy link

changeset-bot bot commented Jan 17, 2023 •
edited

🦋 Changeset detected

Latest commit: 3836097

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

@mattxwang mattxwang mentioned this pull request Jan 17, 2023
Closed
mattxwang
mattxwang commented Jan 25, 2023
View reviewed changes
Copy link
Member Author

@mattxwang mattxwang left a comment •
edited

Choose a reason for hiding this comment

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

Just wrapped up a draft for a first pass on this feature. Broadly, I use a similar approach to @imvetri, except:

  • instead of redefining the properties, I convert the existing longhandSubPropertiesOfShorthandProperties to contain an (ordered) array, and then recreate longhandSubPropertiesOfShorthandProperties by calling the Set constructor - hopefully reduces code de-dup while requiring few code changes outside of this PR
  • then, I use the orders to "replace" the prop + values of the first node where a longhand declaration was used - adjusting @imvetri's original solution
  • I added fixes to each test case, leaning on the MDN docs + CSS spec to make sure my order is correct

I'd like to get feedback on:

  • broadly, does this approach make sense?
  • how many more tests should I write (I am considering one per shorthand)
  • how am I doing with naming? I found the existing code slightly confusing to read through, but also recognize that I've added some more complexity
  • am I doing TS-related stuff properly?

This is certainly a complicated-ish PR and requires a bit of double-checking. I think this PR is ready to review, but I'll certainly do some more rigorous testing (likely adding more test cases) just in case.

I will still write more test cases, but would like some feedback before I wrap them up!

lib/reference/properties.js Outdated Show resolved Hide resolved
lib/reference/properties.js Outdated Show resolved Hide resolved
lib/rules/declaration-block-no-redundant-longhand-properties/index.js
@@ -42,10 +43,13 @@ const rule = (primary, secondaryOptions) => {
return;
}

/** @type {Map<string, string[]>} */
/** @type {Map<string, import('stylelint').ShorthandProperties[]>} */
Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty sure the previous type was slightly incorrect, but could be wrong!

lib/rules/declaration-block-no-redundant-longhand-properties/index.js Outdated Show resolved Hide resolved
lib/rules/declaration-block-no-redundant-longhand-properties/index.js Outdated Show resolved Hide resolved
@mattxwang mattxwang marked this pull request as ready for review January 25, 2023 03:58
@ybiquitous
Copy link
Member

@mattxwang Thanks for creating the pull request, and sorry for the late reply.

The Set doc on MDN says:

You can iterate through the elements of a set in insertion order.

So, is it possible to use longhandSubPropertiesOfShorthandProperties instead of adding orderedLonghandSubPropertiesOfShorthandProperties? Of course, sub properties in longhandSubPropertiesOfShorthandProperties should be sorted to correspond to shorthand properties (you done it already). If possible, it would be no longer needed to have similar named variables (they are a part of the Stylelint public API).

@mattxwang
Uses insertion-order for Set instead of defining `orderedLonghandSu…
1263e9e 
…bPropertiesOfShorthandProperties` explicitly
@mattxwang
Copy link
Member Author

Thanks for the suggestion @ybiquitous, that's a much cleaner way to approach this! I've reverted most of the changes regarding orderedLonghandSubPropertiesOfShorthandProperties in 1263e9e, and the test still pass, etc.

@Mouvedia Mouvedia mentioned this pull request Feb 5, 2023
Closed
Mouvedia
Mouvedia reviewed Feb 5, 2023
View reviewed changes
lib/rules/declaration-block-no-redundant-longhand-properties/index.js Outdated Show resolved Hide resolved
lib/rules/declaration-block-no-redundant-longhand-properties/index.js Outdated Show resolved Hide resolved
lib/rules/declaration-block-no-redundant-longhand-properties/README.md Outdated
@@ -66,6 +66,8 @@ This rule complains when the following shorthand properties can be used:

Flexbox-related properties can be ignored using `ignoreShorthands: ["/flex/"]` (see below).

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

Choose a reason for hiding this comment

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

minor

You could add an unfixable: true, test to illustrate that.

@mattxwang
Copy link
Member Author

mattxwang commented Feb 7, 2023 •
edited

Resolved two quick merge conflicts:

  • the link to the "Options" page / fix subheading has changed since I opened this PR
  • index.d.ts was slimmed down (I re-applied my changes to the current main)

Final question: would we prefer more test cases (unfixable or otherwise)? If so, happy to write them up.

@ybiquitous
Copy link
Member

Final question: would we prefer more test cases (unfixable or otherwise)? If so, happy to write them up.

The current cases look enough, thank you. If a problem is found, then let's add cases.

mattxwang and others added 4 commits February 10, 2023 09:04
@mattxwang @ybiquitous
Apply suggestions from code review
52181fa 
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@mattxwang
Use firstDeclNode everywhere else
e46717b
@mattxwang @ybiquitous
Applies @ybiquitous' (wonderful) suggestion
359e036 
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@mattxwang
add TODO comment for text-decoration-thickness
61a9d61
ybiquitous
ybiquitous approved these changes Feb 10, 2023
View reviewed 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.

Great! LGTM 👍🏼

@mattxwang mattxwang added this pull request to the merge queue Feb 10, 2023
Merged via the queue into main with commit a38641c Feb 10, 2023
@mattxwang mattxwang deleted the autofix-declaration-block-no-redundant-longhand-properties branch February 10, 2023 17:25
@ybiquitous ybiquitous mentioned this pull request Feb 11, 2023
Closed
6 tasks
@Mouvedia Mouvedia mentioned this pull request Oct 14, 2023
Closed
@Mouvedia Mouvedia mentioned this pull request Apr 17, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mouvedia Mouvedia Mouvedia left review comments

@jeddy3 jeddy3 jeddy3 left review comments

@ybiquitous ybiquitous ybiquitous approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add declaration-block-no-redundant-longhand-properties autofix
4 participants
@mattxwang @ybiquitous @Mouvedia @jeddy3