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

Handle spaceless multiplication in unit-* rules #2948

Merged
merged 3 commits into from Oct 12, 2017

Conversation

3 participants
@CAYdenberg
Contributor

CAYdenberg commented Oct 10, 2017

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

#2875

Is there anything in the PR that needs further explanation?

Wrote failing tests but haven't implemented fixes yet.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Oct 10, 2017

Tests look good.

@CAYdenberg CAYdenberg changed the title from WIP: Handle spaceless multiplication in unit-* rules to Handle spaceless multiplication in unit-* rules Oct 11, 2017

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Oct 11, 2017

@CAYdenberg Is this ready for review now?

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Oct 11, 2017

Yes!

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Oct 11, 2017

Is there value in creating a parseValue util that houses this in one place, like we do for parseSelector?

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Oct 11, 2017

I thought about that. We really want the postcss-value-parser to handle * as a separator, and it doesn't. So the work around is just to replace it, but we probably only want to make this replacement in unit rules for fear of breaking something else.

If I look at the four unit-* rules, most of the code is duplicated. I thought about creating some kind unit analyzer utility - maybe walkUnits? So that the individual rules could just recieve units as a callback when found, then check them. Or they could pass a "checking" function into the utility.

This would be a bigger change though, so I think this option is OK since it's just one additional line of code that's duplicated between all four files (although one that doesn't make sense without explanation/comment). But I'll happily implement the more thorough solution if you think it's warranted and have some feedback on how it should be designed.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Oct 11, 2017

This would be a bigger change though, so I think this option is OK

Agreed. Thanks for the write-up too. I think we're OK to go with this, as we can always revisit this if we're so inclined and/or have more time on our hands.

@jeddy3

jeddy3 approved these changes Oct 11, 2017

Thanks!

@jeddy3 jeddy3 merged commit 18a42a9 into master Oct 12, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 95.67%
Details

@jeddy3 jeddy3 deleted the issue-2875 branch Oct 12, 2017

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Oct 12, 2017

  • Fixed: unit-* false positives for spaceless multiplication (#2948).

After two approvals, anyone can merge a PR... including the original PR author. So, feel free to merge if you noticed an unmerged PR when you check-in on stylelint. I just did this one as I generally catch-up on stylelint around this time of day.

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