-
Notifications
You must be signed in to change notification settings - Fork 222
fix(color): improve precision in conversions and add tests #217
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
base: main
Are you sure you want to change the base?
Conversation
Previous code was instead handling rgb(255,255,255) as a specific case, which was useless.
…version These coefficients were cluelessly truncated.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
============================================
+ Coverage 70.81% 71.95% +1.14%
- Complexity 994 996 +2
============================================
Files 49 49
Lines 3169 3170 +1
============================================
+ Hits 2244 2281 +37
+ Misses 925 889 -36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d7d5a4d to
2e46629
Compare
…cision loss Notably, this fixes gray(100) to rgb(255,255,255) instead of rgb(254,254,254).
2e46629 to
666a162
Compare
|
This looks generally good and I'm fine with AI generated tests, as long as they are user-reviewed to make sure they cover the cases as intended. What I'm unhappy with are all the comments usually inserted by AI models. Code should usually be self-documenting, comments should only be required either in doc blocks to expand on the class/function names or inline if there are edge cases which cannot be expressed by the code itself. Can you go over the diff and remove unnecessary comments? |
|
I’ve reviewed these tests, and they’re solid — definitely better than if I had tediously written them myself by hand. Frankly, I could have skipped mentioning they were AI‑generated and nobody would have noticed. As for the comments, I find them really useful to get an efficient overview of the code. For these test classes, I don’t even bother reading the implementation first, I just scan the comments — much more efficient. That said, if you point out specific comments you think should go, I’ll consider removing them. |
7543c74 to
367ab47
Compare
Similar to input 50, input 90 is the only other case that depends on using "255/100" in order to be rounded up instead of down. For these cases, rounding down would yield a different but equally linear progression, but inconsistent with expected calculation results.
367ab47 to
0f46d47
Compare
This fixes #184.
Also added unit tests to cover rgb ↔ cmyk ↔ gray conversions.
Disclaimer: I hate writing tests, so these are fully AI-generated. Though, they work perfectly, and cover all the changes I've made in this PR.