Skip to content
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

Document the deprecation policy and add the deprecate_parameter decorator to deprecate parameters #1160

Merged
merged 28 commits into from
Apr 19, 2021

Conversation

seisman
Copy link
Member

@seisman seisman commented Apr 2, 2021

Description of proposed changes

This is a draft implementation to deprecate a parameter but keep backward compatibility.

For example, the Figure.plot() function has the sizes parameter, but we want to rename it to size, and keep backward compatibility. We can add the deprecate_parameter decorator to the Figure.plot() method:

@deprecate_parameter("sizes", "size", "v0.4.0", "v0.6.0")
def plot():
   ...

It means the old parameter name sizes is deprecated since v0.4.0, and will be removed in v0.6.0.
The new parameter name is size.

When users use sizes, a warning will be raised.

References:

Related to #1120.

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

pygmt/helpers/decorators.py Outdated Show resolved Hide resolved
pygmt/helpers/decorators.py Outdated Show resolved Hide resolved
@@ -43,7 +44,8 @@
t="transparency",
)
@kwargs_to_strings(R="sequence", c="sequence_comma", i="sequence_comma", p="sequence")
def plot(self, x=None, y=None, data=None, sizes=None, direction=None, **kwargs):
@deprecate_parameter("sizes", "size", "v0.4.0", "v0.6.0")
Copy link
Member

Choose a reason for hiding this comment

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

I would use the keyword name here, make it easier to regex match when the time comes to deprecate something. Also, do we want to deprecate this at v0.6.0 or v0.5.0?

Suggested change
@deprecate_parameter("sizes", "size", "v0.4.0", "v0.6.0")
@deprecate_parameter("sizes", "size", "v0.4.0", remove_version="v0.6.0")

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, do we want to deprecate this at v0.6.0 or v0.5.0?

As sizes is heavily used, I'd like to give users more time to change the old names to new names.

pygmt/helpers/decorators.py Outdated Show resolved Hide resolved
@weiji14 weiji14 added the deprecation Deprecating a feature label Apr 5, 2021
@seisman
Copy link
Member Author

seisman commented Apr 6, 2021

/format

pygmt/src/plot.py Outdated Show resolved Hide resolved
@seisman seisman changed the title WIP: Add the deprecate_parameter decorator to deprecate parameters Add the deprecate_parameter decorator to deprecate parameters Apr 6, 2021
@seisman seisman added this to the 0.4.0 milestone Apr 6, 2021
@seisman seisman marked this pull request as ready for review April 6, 2021 06:10
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Looks good, I would add a note about our deprecation policy (and how to use @deprecate_parameter) in the maintainer docs, maybe after #1185 is completed.

pygmt/helpers/decorators.py Show resolved Hide resolved
@seisman
Copy link
Member Author

seisman commented Apr 13, 2021

Ping @GenericMappingTools/pygmt-contributors for reviews and comments on this PR.

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Just some proofread edits. No outstanding issues on the @deprecate_parameter function.

doc/maintenance.md Outdated Show resolved Hide resolved
doc/maintenance.md Outdated Show resolved Hide resolved
doc/maintenance.md Outdated Show resolved Hide resolved
doc/maintenance.md Outdated Show resolved Hide resolved
pygmt/helpers/decorators.py Outdated Show resolved Hide resolved
@core-man
Copy link
Member

Woow~ Looks great 😄 Only some minor suggestions.

Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Yao Jiayuan <[email protected]>
@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Apr 16, 2021
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Apr 19, 2021
@seisman seisman changed the title Add the deprecate_parameter decorator to deprecate parameters Document the deprecation policy and addd the deprecate_parameter decorator to deprecate parameters Apr 19, 2021
@seisman seisman merged commit 3469dfe into master Apr 19, 2021
@seisman seisman deleted the deprecate-decorator branch April 19, 2021 04:21
@weiji14 weiji14 changed the title Document the deprecation policy and addd the deprecate_parameter decorator to deprecate parameters Document the deprecation policy and add the deprecate_parameter decorator to deprecate parameters Oct 19, 2021
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…rator to deprecate parameters (GenericMappingTools#1160)

* Add the deprecate_parameter decorator to deprecate parameters
* Use FutureWarning instead of DeprecationWarning
* Document the deprecation policy

Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Meghan Jones <[email protected]>
Co-authored-by: Yao Jiayuan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants