Skip to content
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

Graceful parsing of Colorants #232

Closed
tbreloff opened this issue Nov 16, 2015 · 4 comments
Closed

Graceful parsing of Colorants #232

tbreloff opened this issue Nov 16, 2015 · 4 comments

Comments

@tbreloff
Copy link
Member

This isn't ideal behavior:

julia> using Colors

julia> parse(Colorant, "red")
RGB{U8}(1.0,0.0,0.0)

julia> parse(Colorant, colorant"red")
ERROR: StackOverflowError:
 in parse at /home/tom/.julia/v0.4/Colors/src/parse.jl:136 (repeats 342 times)

Should there be a better signature in parse:

Base.parse{C<:Colorant}(::Type{C}, desc::AbstractString) = convert(C, parse(Colorant, desc))::C

and/or should there be a "pass-through" option for Colorants:

Base.parse{C<:Colorant}(::Type{C}, c::Colorant) = convert(C, c)
@timholy
Copy link
Member

timholy commented Nov 16, 2015

FYI colorant"red" by itself (without the parse) is all you need. But a pass-through would be great, if you want to submit that.

@tbreloff
Copy link
Member Author

Yes... The issue came up somewhere in Plots, of course I'm still looking for the bug since the full stack trace isn't printed due to the stack overflow. (Is that a bug in julia?)

I'll put together a PR for the signatures I mentioned.

On Nov 16, 2015, at 11:48 AM, Tim Holy notifications@github.com wrote:

FYI colorant"red" by itself (without the parse) is all you need. But a pass-through would be great, if you want to submit that.


Reply to this email directly or view it on GitHub.

@timholy
Copy link
Member

timholy commented Nov 16, 2015

Ugh, StackOverflows shouldn't happen even in cases of user error, but I often forget to watch out for this.

The key technique is probably to change only one argument type at a time. Comparing

function Base.parse(::Type{Colorant}, desc::AbstractString)
and
Base.parse{C<:Colorant}(::Type{C}, desc) = convert(C, parse(Colorant, desc))::C
, I'd predict the StackOverflow should go away if you add ::AbstractString to the declaration of desc in the latter. If this is true, would be good to change this.

@timholy
Copy link
Member

timholy commented Feb 7, 2016

Fixed by #233

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

No branches or pull requests

2 participants