Modify set_ticklabels() to fix counterintuitive behavior of set_ticklabels(get_ticklabels)#2246

[MirandaXM]

Dear all,

I made some changes to set_ticklabels() to solve counterintuitive behavior of set_ticklabels(get_ticklabels) #2246.
Hope it helps, thank you~~

[tacaswell]

Please remove these lines.

[tacaswell]

We don't really have things like this else where in the code base and it is good practice to keep the the code style consistent through out the library.

I think this will also fail the automated pep8 checker.

[tacaswell]

Thank you! I left two comments (one cosmetic, one code-related).

Was there supposed to be a test to go with that image?

[tacaswell]

For reference see http://matplotlib.org/devel/testing.html for an explaination of how to add image tests. For this, we probably only need a png test. Add extensions=['png'] as a kwarg to the decorator (that is something we need to document better).

[tacaswell]

If you are interested, I am happy to work with you to make these changes and get this fix merged.

Don't be discouraged, my first (many) PRs to matplotlib it felt like I re-did every line multiple times.

[MirandaXM]

Hi Thomas,

Thanks for all your great help and tolerance ^. ^ I'm very glad to work with you to fix this bug~~

I have improved the fix solution based on your advice, and I'm working on the test. I will commit them as soon as possible.

Thank you very much!

[MirandaXM]

Hi Thomas,

Thanks a lot for your detailed explanation~~ I have made some changes to the code and add corresponding test, please check it. Thank you very much!

[tacaswell]

This looks great!

One last kind of tricky request, can you use git re-base to re-write history so that the first png was never checked in? Probably the easiest way to do this is to squash everything to a single commit (and then use force-push to push it back to your branch on gh).

The reason to do this is that the repo is already very large and we are doing our best to keep the size from growing more than needed.

[MirandaXM]

Hi Thomas,

I have rewritten the commit history of the issue branch, please check it~~

And thanks for telling me such useful way to do this ^3^ if there are any further improvement I can do, please let me know, thank you very much!

[tacaswell]

Awesome!

One more tiny thing (sorry if it seems like I am dragging this out, I just don't always notice everything the first time through). The docstring should be updated to indicate that it now takes a sequence of string or Text objects.

[MirandaXM]

Hi Thomas,

Thanks a lot for pointing out this issue, I will review my fix work carefully next time~~
I fixed the docstring, and also rewrote the history, so it is still one commit.
If there are any further improvement I can do, please let me know, thank you very much!

[tacaswell]

@MirandaXM Thank you and congratulations on your first MPL contribution! Hopefully this will be the first of many.

And don't worry about missing things, this is why we do code review.

[MirandaXM]

Hi Thomas,

Much appreciate for all your great help~~ It is my pleasure that to do my 1st MPL contribution with your help, it is definitely a great lesson I have taken. I will try to do more and better in future~~

Best regards,
Miranda