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

Add decorator to simplify wrapping of common options #1281

Closed
wants to merge 7 commits into from

Conversation

maxrjones
Copy link
Member

Description of proposed changes

This suggestion adds a decorator for including common options in pygmt methods without needing to include a dictionary pair for each option in use_alias and placeholders for each option in the docstrings. It works by taking a list of the common options available to the method, adding them to the aliases attribute, and inserting placeholders into a {common} placeholder in the docstring.

I have adjusted the blockm* functions to use the new decorator as an example to show that the usage and resulting documentation are the same as before.

I think this would be a helpful addition for two reasons:

  1. It would simplify the completion of wrapping the remaining common options (List of GMT aliases #48) by greatly reducing the number of repeated lines.
  2. It would simplify the wrapping of new modules, because the alias name is taken from COMMON_OPTIONS in pygmt/helpers/decorators.py rather than adding the keyword value pair separately for each module.

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

@weiji14 weiji14 marked this pull request as draft May 20, 2021 23:50
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.

Thanks for starting this Meghan! I have just two discussion points for now. Also converted this PR to draft mode to save on CI resources.

@@ -132,13 +127,10 @@ def blockmean(table, outfile=None, **kwargs):


@fmt_docstring
@use_common(options=["V", "a", "f", "r"])
Copy link
Member

Choose a reason for hiding this comment

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

My first impression is that this reduces some readability. What about using the full name as in @use_common(options=["verbose", "aspatial", "coltypes", "registration"])? Or do you think it's ok to use the short alias as is (I realize that it's easier to lookup the dictionary using COMMON_OPTIONS[option] where option is the short alias, compared to doing a reverse lookup).

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be helpful to get more opinions on this. You are correct that the current implementation is simpler but less readable. Either seems fine to me, but I do not have a great perspective on how problematic the readability could be for future contributors/maintainers who have more experience with PyGMT than GMT.

Comment on lines 133 to -132
R="region",
V="verbose",
a="aspatial",
f="coltypes",
r="registration",
Copy link
Member

Choose a reason for hiding this comment

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

Another question I have is the placement of common options like region (R) and frame (B). The current implementation seems to place them all at the very end after the non-common options. Is the @use_common decorator meant to be used only for parameters like verbose/aspatial/panel/etc placed at the end? Or do we want to use them for other common options like region/frame too?

I guess what I'm trying to ask is when we should use @use_alias vs @use_common 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be an option to write a decorator that sorts the parameters section of the docstrings alphabetically (I would enjoy that, but maybe I am getting carried away here). Otherwise, I guess any common options that should go at the top or need unique descriptions for the given module (e.g., region in blockm*) could use use_alias while less important ones could use use_common.

@maxrjones
Copy link
Member Author

I'm closing this PR since it hasn't gotten a lot of traction. Apart from saving a bit of work, the main motivation was to make sure the common options have consistent alias names. Rather than this solution, we could instead add a check for common option aliases when addressing #256.

@maxrjones maxrjones closed this Jul 16, 2021
@weiji14 weiji14 deleted the common-decorator branch October 28, 2021 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants