Legend shadow color #18668
Legend shadow color #18668
Conversation
|
Perhaps consider making |
Is this a pattern elsewhere? I did consider it, but didn't want to introduce a multiple-type flag if that's something that's not currently in the codebase. |
|
We have lots of multiple type flags. And I agree with @anntzer that this would be an appropriate place for one. |
Do you know how those |
|
@Tranquilled Please see https://matplotlib.org/devel/testing.html#writing-an-image-comparison-test for instructions on how to add a new image test. |
|
however, if you can test that it worked (e.g. by inspecting the state of the Figuure) without adding a new image to the test suite that is preferred (to try and keep the size of the repository down). |
Would it be valid to test the new if block by calling the shadow with the same color as default and seeing if it generates the same image? This keeps the repo size down but it seems like cheating. |
|
That would be a bit cheating. You might be able to do something to try and simulate a legend having a shadow and hijack your self into the draw order, but an image test with a 2x3 grid of default, True, False, {3 colors} maybe the best option here. You can probably use svg as the format which would be pretty small. |
|
@tacaswell @dopplershift I can no longer tell why code coverage fails on this branch. |
|
Don't worry about the coverage failuere, codecov sometimes does strange things. |
|
Can you rebase to remove the reverted commits entirely? |
| legends = { | ||
| 'true': plt.legend(loc="upper left", shadow=True), | ||
| 'false': plt.legend(loc="center left", shadow=False), | ||
| 'default': plt.legend(loc="lower left"), | ||
| 'colorstring': plt.legend(loc="upper right", shadow='red'), | ||
| 'colortuple': plt.legend(loc="center right", shadow=(0.1, 0.2, 0.5)), | ||
| 'colortab': plt.legend(loc="lower right", shadow='tab:cyan') | ||
| } | ||
| for x in legends: | ||
| plt.gca().add_artist(legends[x]) |
There was a problem hiding this comment.
I don't understand this; doesn't plt.legend add the legend to the plot already? Also, use ax.legend instead.
|
@QuLogic If it's all the same to you I'd rather just squash. |
lib/matplotlib/tests/test_legend.py
Outdated
| ax.plot(range(100), label="test") | ||
| with pytest.raises(ValueError, match="color or bool"): | ||
| x = plt.legend(loc="upper left", shadow="aardvark") | ||
| fig.canvas.draw() |
There was a problem hiding this comment.
There's no need for this line, as the legend call would have raised.
There was a problem hiding this comment.
Don't think that's the case, the tests all fail.
There was a problem hiding this comment.
Oh, I didn't realize your checks are in draw; that's the wrong place. The setting should be validated in __init__ so that raising occurs when the legend is created, not at some random time in the future.
|
Ping? Tests are not passing, and there are file conflicts. |
PR Summary
Allows the
shadowlegend flag to be a color as well as a bool.If it's a bool, the behavior is unchanged.
I'm unsure whether it would be best to apply the "shadow color transformation" to the specified color or leave it up to the user to decide.
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsandpydocstyle<4and runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).