Skip to content

Conversation

@vlakoff
Copy link
Contributor

@vlakoff vlakoff commented Nov 25, 2025

  • Fixes a division by zero when converting rgb(0,0,0) to cmyk
  • Improves precision in several places, by using:
    • corrected luminance coefficients
    • integer-based calculations to avoid floating-point precision loss
    • rounding instead of implicit truncation

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.

Previous code was instead handling rgb(255,255,255) as a specific case, which was useless.
…version

These coefficients were cluelessly truncated.
@codecov
Copy link

codecov bot commented Nov 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.95%. Comparing base (c2fe9b3) to head (0f46d47).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vlakoff vlakoff force-pushed the fix/color-conversion branch from 2e46629 to 666a162 Compare November 26, 2025 00:41
@DASPRiD
Copy link
Member

DASPRiD commented Nov 26, 2025

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?

@DASPRiD DASPRiD self-assigned this Nov 26, 2025
@vlakoff
Copy link
Contributor Author

vlakoff commented Nov 26, 2025

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.

@vlakoff vlakoff force-pushed the fix/color-conversion branch from 7543c74 to 367ab47 Compare November 26, 2025 19:45
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.
@vlakoff vlakoff force-pushed the fix/color-conversion branch from 367ab47 to 0f46d47 Compare November 26, 2025 19:55
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.

Gray::toRgb rounding error

2 participants