-
Notifications
You must be signed in to change notification settings - Fork 47
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
Modify strictness of color parser (#357) #360
Conversation
- Modify white space handling - Add error message about 8-digit or 4-digit hex notation - Add support for some CSS4 notations - Add tests - Speed up parsing averagely
It is not difficult to implement the 8-digit or 4-digit hex notation, but we should settle the issue #353. |
Codecov Report
@@ Coverage Diff @@
## master #360 +/- ##
==========================================
+ Coverage 76.23% 76.98% +0.75%
==========================================
Files 10 10
Lines 808 817 +9
==========================================
+ Hits 616 629 +13
+ Misses 192 188 -4
Continue to review full report at Codecov.
|
@test_throws ErrorException parse(Colorant, "#FB0A") | ||
@test_throws ErrorException parse(Colorant, "#BAD05") | ||
@test_throws ErrorException parse(Colorant, "#BAD0007") | ||
@test_throws ErrorException parse(Colorant, "#FFBB00AA") # not supported yet |
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.
In such cases is @test_broken
more appropriate?
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.
That's right, if you (we) have already specified the correct behavior. Throwing the "not supported" error is not a broken behavior now. (Of course, checking the error message might be better.)
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.
I'm fine with it either way, no reason not to merge.
If this PR causes a problem (e.g. "not supported" error), it is probably a potential bug in the user scripts. |
Aww, I'm sorry but I mistook this PR number for the issue number. |
This fixes #357.