FIX patch.update_from to also copy _original_edge/facecolor #10575
Conversation
fix for bug described in matplotlib#10574
|
Actually can you add a small test to check that uodate_property keeps doing the right thing? Need not use legend or have an image test |
|
Suggest adding to def test_update_from():
# test that update_from properly sets the facecolor.
fig, ax = plt.subplots()
rect = Rectangle((0, 0), 1., 1., facecolor='magenta')
rect2 = Rectangle((1., 0), 1., 1., facecolor='blue')
ax.add_patch(rect)
ax.add_patch(rect2)
rect2.update_from(rect)
rect2.set_alpha(0.8)
assert rect2.get_facecolor() == (1., 0., 1., 0.8)I tried force pushing, but.... |
|
@KonradAdamczyk any chance you can add the test? If not, is it ok to force push onto your PR? |
|
@jklymak |
|
@KonradAdamczyk were you going to add the test, or would you like me to? |
|
@jklymak I already added the test. |
|
It needs to be in the same git branch as this PR |
|
ping @KonradAdamczyk |
|
Great, thanks @KonradAdamczyk ! I'm sure this will attract a second reviewer soon! |
|
The test should go in |
|
Ooops, jeez. Good catch @QuLogic |
test set_alpha after update_from changes only alpha in color
this test is moved to lib/matplotlib/tests/test_patches.py
lib/matplotlib/tests/test_patches.py
Outdated
| # given | ||
| source_facecolor = (0.234, 0.123, 0.135, 0.322) | ||
| source_egdecolor = (0.728, 0.682, 0.945, 0.268) | ||
| source = mpatches.Rectangle((0, 0), 1., 1., facecolor=source_facecolor, edgecolor=source_egdecolor) |
There was a problem hiding this comment.
Could you make this line and the one below 79 characters or less long by putting some of the arguments on the next line (and lining them up with the brackets?). Otherwise this looks good to go 👍
lib/matplotlib/tests/test_patches.py
Outdated
| def test_when_update_from_and_set_alpha_then_only_alpha_changes(): | ||
| # given | ||
| source_facecolor = (0.234, 0.123, 0.135, 0.322) | ||
| source_egdecolor = (0.728, 0.682, 0.945, 0.268) |
|
@dstansby can you approve request now ? |
1 similar comment
|
@dstansby can you approve request now ? |
|
This still has flake8 errors... |
|
I should have just fixed the PEP8 issues, lets see if travis agrees! |
fix for bug described in
#10574
PR Summary
PR Checklist