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

Fix 1825 #1831

Merged
merged 6 commits into from Feb 24, 2016

Conversation

Projects
None yet
3 participants
@chaosphere2112
Contributor

chaosphere2112 commented Feb 16, 2016

Fixes the issue discovered by Ram on the mailing list, where you could not set the missing color for a graphics method using a color's name. Adds a test to cover that usecase.

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Feb 16, 2016

try:
# Is it a colormap index?
return colormap.index[color]
except ValueError:

This comment has been minimized.

@aashish24

aashish24 Feb 17, 2016

Contributor

should be a IndexError?

This comment has been minimized.

@chaosphere2112

chaosphere2112 Feb 17, 2016

Contributor

Nope, it's a ValueError. We implement __setitem__ and __getitem__ on the colormap object, and raise a ValueError if the key isn't a number between 0 and 256.

This comment has been minimized.

@aashish24

aashish24 Feb 17, 2016

Contributor

Interesting, I would expect a IndexError from this code but your code looks go to me.

@@ -1716,6 +1716,37 @@ def getworldcoordinates(gm, X, Y):
return wc
def rgba_color(color, colormap):

This comment has been minimized.

@aashish24

aashish24 Feb 17, 2016

Contributor

Could you please add some comments on this code behavior?

int(c)
except:
break
else:

This comment has been minimized.

@aashish24

aashish24 Feb 17, 2016

Contributor

is it just me or this else is not aligned?

This comment has been minimized.

@chaosphere2112

chaosphere2112 Feb 17, 2016

Contributor

No, the else is for the for loop; it executes if the code doesn't break.

This comment has been minimized.

@aashish24

aashish24 Feb 17, 2016

Contributor

I see. Using try and except like this is consider best practice?

This comment has been minimized.

@chaosphere2112

chaosphere2112 Feb 17, 2016

Contributor

Yup, exceptions are cheap in python. Ask for forgiveness, not permission

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Feb 17, 2016

LGTM except that I have question on best practices for try and except.

+2

doutriaux1 added a commit that referenced this pull request Feb 24, 2016

@doutriaux1 doutriaux1 merged commit a6757f5 into master Feb 24, 2016

5 of 6 checks passed

cont-int/LLNL/Linux-RH6 (FULL) 'ctest -j5 -D Experi' (Tue Feb 16 16:47:00 2016)
Details
cont-int/LLNL/Linux-RH6 (MESA) 'ctest -j12 -D Exper' (Tue Feb 16 16:07:21 2016)
Details
cont-int/LLNL/Linux-Ub. 15.10 (FULL/MESA) 'git reset --hard ori' (Tue Feb 16 14:47:53 2016)
Details
continuous-integration/kitware-buildbot/uvcdat-garant-linux-release/ Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@doutriaux1 doutriaux1 deleted the fix_1825 branch Feb 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment