Skip to content

Fix Numeric parsing #109

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

Merged
merged 2 commits into from
Apr 17, 2020
Merged

Fix Numeric parsing #109

merged 2 commits into from
Apr 17, 2020

Conversation

jonathantneal
Copy link
Collaborator

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

This changes the parser algorithm for Numerics to follow the CSS Syntax specification for Numeric Tokens.

This resolves various issues I am having, where numbers as plain as 32deg or as modern as 1138--hex are considered words by this tool.

The code is not lifted from any other tool that I know of, but written by me from my own RegExp implementation of the CSS Syntax, which I have yet to finish or release, and maybe never will.

If fortune allows you to work on OSS, I would greatly appreciate an updated patch or minor release. This PR does remove the internal parseUnit and testUnit static methods, but those methods were undocumented, and both the test and fromTokens methods continue to work as expected.

@shellscape
Copy link
Owner

Good fortune indeed. We've got a work week that allows me time for OSS and I've been catching up.

Given your notes, I'd like to be safe and do a new major version. Do you think that's reasonable, or will that present too large a barrier for upgrading on your end?

@jonathantneal
Copy link
Collaborator Author

Yes, I will gladly perform a major update to the affected plugins. Also, I believe your "todos" in the numeric tests can now be added. I only did not want to muddy my PR.

@jonathantneal
Copy link
Collaborator Author

Hey, anything I can do to help move this along? If you’ve been busy, that’s understandable, and so this only applies if there is anything I can do in this PR or through other PRs.

@shellscape
Copy link
Owner

We'll get this in today. Check out that CI error in the meantime.

Copy link
Owner

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

Great stuff. This is going to make a lot of folks happy.

@shellscape
Copy link
Owner

Looks like the version of node that the test container was using didn't like the Ava upgrade. I've updated the container but not sure how long that'll take to update. I'll rerun the tests when that happens.

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

Successfully merging this pull request may close these issues.

2 participants