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.

Sign up for GitHub

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

Jump to bottom

improve warning when nonunique colors used with manual_color_key #1172

Merged
merged 1 commit into from
Jul 1, 2018
Merged

improve warning when nonunique colors used with manual_color_key #1172

merged 1 commit into from
Jul 1, 2018

Conversation

tlnagy
Copy link
Member

@tlnagy tlnagy commented Jun 27, 2018

fixes #1170

@tlnagy
Copy link
Member Author

tlnagy commented Jun 27, 2018

Looks like tests are segfaulting on Travis. Not sure why. Tests are also segfaulting on

Gadfly.jl/test/testscripts/issue1125.jl

Line 23 in f922f4d

vstack(plots...)
locally for me on both this branch and master.

@bjarthur
Copy link
Member

the travis errors are similar to #1166. i wonder whether it would happen with julia 0.6.2.

i can confirm the segfault with issue1125.jl locally.

bjarthur
bjarthur reviewed Jun 28, 2018
View reviewed changes
src/guide.jl Outdated
for (c, l) in zip(colors, labels)
if c in keys(labeldict)
error("Colors should not appear more than once")
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haskey(labeldict,c) || error() would be more succint. otherwise, lgtm once CI passes.

@tlnagy
Copy link
Member Author

tlnagy commented Jun 28, 2018

It segfaults on issue1125.jl on v0.6.2 too so we must've changed something on master that triggered breakage in Base. We need to track this down.

@bjarthur
Copy link
Member

#1123 might be related

@bjarthur
Copy link
Member

worth noting that issue1125.jl works for me when run all by itself using include("test/testscripts/issue1125.jl"). the failure only occurs for Pkg.test("Gadfly"). so there must be something about the sequence of events leading up to it.

@bjarthur
Copy link
Member

JuliaLang/julia#27607 seems to have the same error, and the fix is scheduled to be backported to 0.6.4.

@tlnagy tlnagy force-pushed the tn/better-warn-nonunique-color branch from 749527e to 10534a8 Compare July 1, 2018 18:39
@tlnagy
Copy link
Member Author

tlnagy commented Jul 1, 2018

It looks like tests on v0.6 are passing on master again so I've updated this PR and hopefully CI passes this time.

@tlnagy
Copy link
Member Author

tlnagy commented Jul 1, 2018

Looks like they're passing now!

@tlnagy tlnagy merged commit 3071ad6 into GiovineItalia:master Jul 1, 2018
@tlnagy tlnagy deleted the tn/better-warn-nonunique-color branch July 1, 2018 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjarthur bjarthur bjarthur left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

manual_color_key with non-unique colours throws unhelpful error
2 participants
@tlnagy @bjarthur