Skip to content

Decorator for deleting a parameter with a deprecation period. #13173

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.

Sign up for GitHub

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
Merged
merged 1 commit into from
Feb 7, 2019
Merged

Decorator for deleting a parameter with a deprecation period. #13173

merged 1 commit into from
Feb 7, 2019
+85 −6

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 13, 2019 •
edited
Loading

PR Summary

As an example application, deprecate the unused shape and imlim args to
imshow() (unused since around 4d1107c (2006)).

Goes on top of #13128 to avoid a rebase. [edit: it has been merged]

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
Copy link
Member

I understand your motivation to go on top of another open PR. However, that makes review more difficult. I'm waiting with the review until #13128 is in.

Semi-OT: I've the feeling that PRs pile up. The number is up again to close to 300 (we got it down to 250 some time ago). Also, there are a number of "ready" PRs: 17 open ones with "needs review" and 53 open ones with at least one approval. I assume many of them could go without further modification, they just need the formal reviews. However, no idea how we get the reviews up. 🙁

@anntzer anntzer mentioned this pull request Jan 13, 2019
Merged
6 tasks
@anntzer
Copy link
Contributor Author

anntzer commented Jan 14, 2019 •
edited
Loading

No hurries.

I'd like to propose a new merge rule to move in uncontroversial PRs faster, something like

If a PR has one positive review and no negative review since at least two weeks [time up to bikeshedding], a core dev (typically either the reviewer, or the author if he's a core dev) can propose to merge it on the basis of a single-review; this should be done with a message pinging all devs and tagging with a "merge-with-single-review" label; then any dev can either do a proper review, or even just say "I think this needs further review", but if no one makes any objection within another two weeks, then the PR can be merged with that single review. To avoid overwhelming the system, any dev can only propose one PR for "single-review-merge" at a time.

To be discussed...

@anntzer anntzer force-pushed the delete_parameter branch 2 times, most recently from a17ff6c to e90d264 Compare January 14, 2019 10:06
@anntzer anntzer mentioned this pull request Jan 14, 2019
Merged
6 tasks
@anntzer
Copy link
Contributor Author

anntzer commented Feb 2, 2019

rebased

jklymak
jklymak approved these changes Feb 7, 2019
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.

I don't fully parse the functools magic, but this seems to do what is said.

@jklymak
Copy link
Member

jklymak commented Feb 7, 2019

Anyone can merge after rebase and passing tests....

@anntzer
Decorator for deleting a parameter with a deprecation period.
c62f491 
As an example application, deprecate the unused shape and imlim args to
imshow() (unused since around 4d1107c (2006)).
@anntzer
Copy link
Contributor Author

anntzer commented Feb 7, 2019

rebased

@jklymak jklymak merged commit 32ecf08 into matplotlib:master Feb 7, 2019
@anntzer anntzer deleted the delete_parameter branch February 7, 2019 18:19
@QuLogic QuLogic added this to the v3.1.0 milestone Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@anntzer @timhoffm @jklymak @QuLogic