-
-
Notifications
You must be signed in to change notification settings - Fork 927
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
Fix declaration-block-no-redundant-longhand-properties
autofix conflicts
#7626
Conversation
🦋 Changeset detectedLatest commit: 093fd05 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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/declaration-block-no-redundant-longhand-properties/index.mjs
Outdated
Show resolved
Hide resolved
lib/rules/declaration-block-no-redundant-longhand-properties/__tests__/index.mjs
Outdated
Show resolved
Hide resolved
lib/rules/declaration-block-no-redundant-longhand-properties/__tests__/index.mjs
Outdated
Show resolved
Hide resolved
declaration-block-no-redundant-longhand-properties
autofix conflictsdeclaration-block-no-redundant-longhand-properties
autofix conflicts for border-*
2c3d386
to
91fdd09
Compare
I notice that this problem is about
|
Apart from
which might be covered by #7609 in a particular case: a {
border-top-width: foo;
border-right-width: foo;
border-bottom-width: foo;
border-left-width: foo;
border-top-color: bar;
border-right-color: bar;
border-bottom-color: bar;
border-left-color: bar;
border-top-style: qux;
border-right-style: qux;
border-bottom-style: qux;
border-left-style: qux;
} the rest looks OK.
Just think about it, the detection of the shorthand possibilities for |
declaration-block-no-redundant-longhand-properties
autofix conflicts for border-*
declaration-block-no-redundant-longhand-properties
autofix conflicts
I understood this PR's fix cannot resolve the false positives. Honestly, I think we should reconsider the rule's implementation, not adding a workaround patch, because this rule has been fixed at many times recently. |
@ybiquitous I have added an |
@Mouvedia I'm against adding this workaround, and again we should reconsider the implementation even if it would be a big change. Honestly, I don't want to review PRs so many times only for one rule, and this workaround may occur other problems. In addition, this bug has been not found for a long time. You say this is "major", but I guess many people are not in trouble. I think it's a good timing to detect the rule's better implementation design. |
Ill just explain what this fix does then, maybe that will change your mind. I hope that's clearer and I managed to convince you. |
9214ccd
to
752f816
Compare
752f816
to
03c005f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I agree with merging this since it resolves at lease autofix problem. But I still have the opinion to reconsider this rule's implementation as soon as possible.
If you fix the changelog, you can merge. LGTM.
This is a legitimate worry. Having 2 rules depending on the sets and having to juggle them both every time they are changed can be considered daunting. If we were to take a perf approach for the refactor I think @romainmenke might be interested by it based on his work on #6869.
I will fix the changelog. |
Closes #7610
No, it's self-explanatory.