Deprecate unused testing cleanup mechanisms #17042

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

Jump to bottom

Closed

SidharthBansal wants to merge 4 commits into matplotlib:master from SidharthBansal:cleanup

Closed

Deprecate unused testing cleanup mechanisms #17042

SidharthBansal wants to merge 4 commits into matplotlib:master from SidharthBansal:cleanup

Conversation

SidharthBansal

Copy link

Contributor

@SidharthBansal SidharthBansal commented Apr 6, 2020

PR Summary

The class matplotlib.testing.decorators.CleanupTestCase and the decorator matplotlib.testing.decorators.cleanup are not used in the code base. We have an automatic cleanup via an auto-used fixture instead. This PR removes them.
Fixes #14329

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@timhoffm @SidharthBansal


 Cleanups to contour docs.

4bb74b8

@SidharthBansal SidharthBansal changed the title ~~(削除) Cleanups to contour docs. (削除ここまで)~~ (追記) Deprecate unused testing cleanup mechanisms (追記ここまで)

Apr 6, 2020

@SidharthBansal


 Merge branch 'master' of https://github.com/matplotlib/matplotlib int...

88cfd28

...o cleanup

@SidharthBansal SidharthBansal force-pushed the cleanup branch from b05f3db to 88cfd28 Compare

April 6, 2020 09:42

@SidharthBansal


 Updated deprecation.rst for next release

13e2cbd

@SidharthBansal

Copy link

Contributor Author

SidharthBansal commented Apr 6, 2020 •

edited

Loading

@jklymak @story645 kindly review and please tell me things which I am missing here.

@SidharthBansal


 rebase

a61f71b

@SidharthBansal SidharthBansal force-pushed the cleanup branch from 901685a to a61f71b Compare

April 7, 2020 11:55

jklymak

jklymak requested changes

Apr 7, 2020

View reviewed changes

Copy link

Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the deprecation decorators.

lib/matplotlib/testing/decorators.py

@@ -35,58 +35,6 @@ def _cleanup_cm():

plt.close("all")

class CleanupTestCase(unittest.TestCase):

Copy link

Member

@jklymak jklymak Apr 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a deprecation, you have just removed these.

What is the motivation for deprecating these? Just general cleanup?

@jklymak

Copy link

Member

jklymak commented Apr 7, 2020 •

edited

Loading

I take #14329 (comment) to mean that @anntzer would explicitly like to keep these methods.

@SidharthBansal

Copy link

Contributor Author

SidharthBansal commented Apr 7, 2020 via email •

edited

Loading

I am currently learning deprecations here. Earlier, in deprecations at other orgs, we remove that code OR use another method/class that is more robust instead of the current one. So, there is tech stack difference too. So, I am currently learning about decorators and wrappings. :-) I will deprecate them by decorators.

...

On Tue, Apr 7, 2020 at 9:34 PM Jody Klymak ***@***.***> wrote: I take #14329 (comment) <#14329 (comment)> to mean that @Annzter <https://github.com/Annzter> would explicitly like to keep these methods. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#17042 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFAAEQ3GOHLHXBVCLB26KZTRLNFHHANCNFSM4MCDU4LQ> .

@anntzer

Copy link

Contributor

anntzer commented Apr 7, 2020 •

edited

Loading

I think these can/should stay for now.

@SidharthBansal Matplotlib is a very widely used library, and effectively anything that we expose is likely used by tons of people regardless of whether we intended it for external use or not. Thereforce we try not to remove stuff unless there's a clear reason to ("clear" being something that you can only evaluate with some experience...) and even then, we need a deprecation period where the thing stays but third-party users who rely on it get a warning that the API is going away.

In this specific case, I think this API has a legitimate use case and is self-contained enough that it does not cause undue maintenance burden by staying. Obviously @timhoffm had a different opinion in #17042 and may therefore want to chime in.

@SidharthBansal

Copy link

Contributor Author

SidharthBansal commented Apr 7, 2020 via email

we need a deprecation period where the thing stays but third-party users

who rely on it get a warning that the API is going away. That is a new thing I learnt today. Thanks for thorough explanation. So, shall I deprecate it OR we are aiming at closing the PR? If we are willing to close the PR, my humble suggestion will be to attach `wont-fix` type label(whatsoever used here) at respective issue so that no one else will attempt to submit a pr for it until we necessarily need it or simply close the issue. Thanks for helping me. You all are very helpful :-)

...

On Tue, Apr 7, 2020 at 10:20 PM Antony Lee ***@***.***> wrote: I think these can/should stay for now. @SidharthBansal <https://github.com/SidharthBansal> Matplotlib is a very widely used library, and effectively anything that we expose is likely used by tons of people regardless of whether we intended it for external use or not. Thereforce we try not to remove stuff unless there's a clear reason to ("clear" being something that you can only evaluate with some experience...) and even then, we need a deprecation period where the thing stays but third-party users who rely on it get a warning that the API is going away. In this specific case, I think this API has a legitimate use case and is self-contained enough that it does not cause undue maintenance burden by staying. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17042 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFAAEQ6OVNRN6RU43HG3XATRLNKU5ANCNFSM4MCDU4LQ> .

@anntzer

Copy link

Contributor

anntzer commented Apr 7, 2020

My opinion is to close as wontfix, but again let's give @timhoffm time to give his opinion as well. Or sometimes issues just need more discussion :)

@jklymak

Copy link

Member

jklymak commented Apr 7, 2020

I think we can proactively close this. If @timhoffm wants more discussion, he can re-open.

@jklymak jklymak closed this

Apr 7, 2020

@jklymak jklymak added the status: won't or can't fix label

Apr 7, 2020

@SidharthBansal

Copy link

Contributor Author

SidharthBansal commented Apr 7, 2020 via email

Agreed!

...

On Tue, Apr 7, 2020 at 10:53 PM Antony Lee ***@***.***> wrote: My opinion is to close as wontfix, but again let's give @timhoffm <https://github.com/timhoffm> time to give his opinion as well. Or sometimes issues just need more discussion :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17042 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFAAEQ6BUUPATCSD5PQHYBTRLNOSPANCNFSM4MCDU4LQ> .

@timhoffm

Copy link

Member

timhoffm commented Apr 7, 2020 •

edited

Loading

@anntzer I'm fine with keeping if you think it's worth it.

Labels

status: won't or can't fix

4 participants

@SidharthBansal @jklymak @anntzer @timhoffm

Uh oh!

Deprecate unused testing cleanup mechanisms #17042

Deprecate unused testing cleanup mechanisms #17042

Uh oh!

Conversation

@SidharthBansal SidharthBansal commented Apr 6, 2020

PR Summary

PR Checklist

Uh oh!

SidharthBansal commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@jklymak jklymak left a comment

Choose a reason for hiding this comment

Uh oh!

@jklymak jklymak Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

jklymak commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SidharthBansal commented Apr 7, 2020 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anntzer commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SidharthBansal commented Apr 7, 2020 via email

Uh oh!

anntzer commented Apr 7, 2020

Uh oh!

jklymak commented Apr 7, 2020

Uh oh!

SidharthBansal commented Apr 7, 2020 via email

Uh oh!

timhoffm commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

SidharthBansal commented Apr 6, 2020 •

edited

Loading

jklymak commented Apr 7, 2020 •

edited

Loading

SidharthBansal commented Apr 7, 2020 via email •

edited

Loading

anntzer commented Apr 7, 2020 •

edited

Loading

timhoffm commented Apr 7, 2020 •

edited

Loading