JuliaGraphics / Colors.jl

Color manipulation utilities for Julia
Other
200 stars 44 forks source link

fix union{} ambiguity #527

Closed Roger-luo closed 1 year ago

Roger-luo commented 1 year ago

See https://github.com/JuliaLang/julia/issues/47840

codecov[bot] commented 1 year ago

Codecov Report

Base: 94.33% // Head: 94.09% // Decreases project coverage by -0.23% :warning:

Coverage data is based on head (26b93f7) compared to base (b550b7b). Patch has no changes to coverable lines.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #527 +/- ## ========================================== - Coverage 94.33% 94.09% -0.24% ========================================== Files 9 9 Lines 1270 1270 ========================================== - Hits 1198 1195 -3 - Misses 72 75 +3 ``` | [Impacted Files](https://codecov.io/gh/JuliaGraphics/Colors.jl/pull/527?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics) | Coverage Δ | | |---|---|---| | [src/display.jl](https://codecov.io/gh/JuliaGraphics/Colors.jl/pull/527/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-c3JjL2Rpc3BsYXkuamw=) | `100.00% <ø> (ø)` | | | [src/utilities.jl](https://codecov.io/gh/JuliaGraphics/Colors.jl/pull/527/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-c3JjL3V0aWxpdGllcy5qbA==) | `95.60% <0.00%> (-0.74%)` | :arrow_down: | | [src/conversions.jl](https://codecov.io/gh/JuliaGraphics/Colors.jl/pull/527/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-c3JjL2NvbnZlcnNpb25zLmps) | `99.13% <0.00%> (-0.29%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

Roger-luo commented 1 year ago

I'm wondering if someone could review this? The test coverage drop does not seem related to this PR.

johnnychen94 commented 1 year ago

will make a release later

aplavin commented 1 year ago

@Roger-luo @johnnychen94 this PR doesn't have any tests to actually confirm the fix, and AFAIK the fix doesn't work. Union{} <: T <: Color is exactly the same as T <: Color.

Roger-luo commented 1 year ago

@aplavin yeah I think you are right, I just realize Union{} <: T is always true, thus this won't work unless we type pirate Union{} this MIME type.

But this does not work for show since lots of backends will call the corresponding show by checking if there is a valid show overloaded for the richest MIME type, e.g https://github.com/fonsp/Pluto.jl/blob/0f0b6f596fd4cf27b2665e1d93ddd44d96fe2a99/src/runner/PlutoRunner.jl#L1148 as a result we will see MIME"text/plain" displayed as picture if we overload

MIME"image/svg+xml" => MIME"text/plain"

I don't have a good way to fix this now.

aplavin commented 1 year ago

I agree, and think this PR should be reverted so that not to confuse future readers.

johnnychen94 commented 1 year ago

@aplavin Thanks for identifying this. I didn't think carefully when merging it -- was checking it on the phone and did a quick merge. Reverted in 9518578

This Union{} would be a nightmare to JuliaImages. Almost every function is written in such a way.

johnnychen94 commented 1 year ago

I realize that I shouldn't make the release on the master branch since there are plenty of breaking changes made by @kimikage since v0.12.8. Thus I've made a backport release branch https://github.com/JuliaGraphics/Colors.jl/tree/release-0.12 with a new patch release v0.12.10 to supersede v0.12.9.

v0.12.10 includes #521 (@brenhinkeller) and #525(@williamjsdavis) as bug fixes.

I've also submitted a yank patch for v0.12.9 to General https://github.com/JuliaRegistries/General/pull/74192