Change dictionary to list of tuples to permit duplicate keys #19666
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
|
Have not reviewed this, but am notionally in favor of it 👍 |
| if type(label) == tuple: | ||
| label = label[0] |
There was a problem hiding this comment.
This makes the function somewhat awkward. In such a case, you should at least document what label can be.
But checking the code, it turns out that cmp_key() is only used another time for mappabledict. It suffers from the same duplicate label problem and should be changed to a tuple list as well. This will then make the type distinction unnecessary.
There was a problem hiding this comment.
Thanks for the comment, I will do it.
|
Thanks @PhilipSchiff! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again. |
Thank you, and thanks Tim for the guidance. I hope to continue as well. |
PR Summary
As described in #19607, when multiple curves are given the same label, only the first one of each redundant label can be edited in the PyQT backend. This is because the curves and their corresponding labels have been saved in a dictionary. It's possible that a user may have a use case for giving multiple curves the same label, so they should be able to edit them separately as well.
To accommodate this, we modified the dictionary which was saving the labeled curves, replacing it with a list of tuples. The list of tuples is sorted in the same way.
I am not sure how to add the tests for this; so far I just tested it manually creating an instance of
NavigationToolbar2QTand giving it redundantly named curves, and verify that they can be edited independently.There is a test at
lib/matplotlib/tests/test_backend_qt.py::test_figureoptions, but all it does is call a mock which will eventually callfigureoptions.figure_edit, just creating it without checking the number of curves available to edit, seems insufficient.PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand 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).