-
Notifications
You must be signed in to change notification settings - Fork 36
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
Support conversion to a StyledStrings.SimpleColor #293
Conversation
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.
LGTM, with one small tweak.
This enables easy interoperability with the Face constructor of StyledStrings.
Thanks! |
This enables easy interoperability with the Face constructor of StyledStrings.
I have no intention to oppose this PR, but I think |
There's no colour parsing going on though? |
I used "parsing" in an abstract sense. Sorry for the confusion. |
This enables easy interoperability with the Face constructor of StyledStrings.
This reverts commit 0c13d0c.
Keeping ColorTypes as small as possible is a good comment; I've seen people thinking 0.2s loading time is too large (#270) From the engineering and maintenance-ship part, if some codes already sit here and there's no "obvious" impact on the user experiences, there's no need to take the risk for the re-factoring unless someone is willing to respond to whatever happens during the transferring. Since this PR has only been merged and has not yet been released, I'm okay with either ColorTypes or Colors if anyone wants to make the change. The real problem with this PR is that StyledStrings is a stdlib package in Julia 1.11+, and this makes it impossible for ColorTypes to register a new version if we want to maintain backward compatibility (5eea76d#comments). @tecosaur I'm not sure what the best solution to this is. Maybe StyledStrings should be registered in the General as well? |
@kimikage the way I see it, support for using Doing it in @johnnychen94 Keeping ColorTypes small and focused is indeed good, but given the simplicity of this specialisation, I'd be amazed to hear if this has any appreciable effect on codeimage size, or load time. Seperately, thinking of a "where does this functionality best sit", other than the usual "not doing type piracy is good", Colors.jl seems a lot more complex than this functionality is.
This shouldn't be a problem for much longer. StyledStrings has a I think we're down to a single blocker in that regard, and that's waiting for |
I still think Colors.jl is a more appropriate place than ColorTypes.jl (of course than StyledStrings.jl too), but is there another option, e.g. via a more unambiguous type such as |
I can't say I actually know what you mean by this. |
Here is the code for the concept (which is by no means practical) # ColorTypes.jl side
using ColorTypes
Base.Tuple(c::Colorant{T, 3}) where {T} = (comp1(c), comp2(c), comp3(c))::NTuple{3, T} (cf. PR #260) # StyledStrings.jl side
using StyledStrings
Base.convert(::Type{StyledStrings.SimpleColor}, c) = StyledStrings.SimpleColor((round.(Int, Tuple(c) .* 255))...)
Base.convert(::Type{StyledStrings.SimpleColor}, c::StyledStrings.SimpleColor) = c julia> col = RGB(1, 0.5, 0)
RGB{Float64}(1.0,0.5,0.0)
julia> styled"{(fg=$col):colored}"
"colored" |
Hmmm, this seems built on too many assumptions for me to feel comfortable with it at a glance. |
I understand your concern. However, are you making any assumptions? julia> using ColorTypes, StyledStrings
julia> hsv = HSV(300, 0.4, 0.5)
HSV{Float64}(300.0,0.4,0.5)
julia> styled"{(fg=$hsv):Color conversion is implemented in Colors.jl}"
ERROR: No conversion of HSV{Float64}(300.0,0.4,0.5) to RGB24 has been defined If this PR is for package developers other than stdlib, they should know what type of colors they are dealing with. |
Also, if in the future julia users request support for the X11 or CSS color specifications, should we add another extension to Colors.jl as well? Of course, having extensions is not so bad in itself. |
Since this PR has already been merged, this discussion is somewhat ”closed" in terms of discoverability in GitHub. |
Frankly, I'm not sure there's much value in continuing it 😅. I still think #293 (comment) sums up the situation well.
To me, this is behaving exactly as expected, with
I don't really follow, are you thinking of a future where somehow X11/CSS named colour specifications get integrated with the Julia stdlib via some dedicated type? This seems like rather extreme speculation. |
JuliaLang/StyledStrings.jl#33 is not the issue you filed? That's all! Thank you for listening to my gripe. (BTW, wouldn't it be better if the extension name was globally unique? |
Yep, it just (1) wouldn't need any changes to
I don't mind a bit of griping 🙂
I don't see why... |
xref: #299 (comment) This PR on its own causes problems with "Project.toml", so I labeled it |
I don't think this is breaking? |
If you have any objections, please comment at #299. |
This enables easy interoperability with the Face constructor of StyledStrings.
With this, it's easy to do fun things like: