Add decorator to simplify wrapping of common options #1281
Add decorator to simplify wrapping of common options #1281
Conversation
weiji14
left a comment
There was a problem hiding this comment.
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.
|
|
||
|
|
||
| @fmt_docstring | ||
| @use_common(options=["V", "a", "f", "r"]) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| R="region", | ||
| V="verbose", | ||
| a="aspatial", | ||
| f="coltypes", | ||
| r="registration", |
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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.
|
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. |
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_aliasand 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:
COMMON_OPTIONSinpygmt/helpers/decorators.pyrather than adding the keyword value pair separately for each module.Reminders
make formatandmake checkto make sure the code follows the style guide.doc/api/index.rst.Slash Commands
You can write slash commands (
/command) in the first line of a comment to performspecific operations. Supported slash commands are:
/format: automatically format and lint the code/test-gmt-dev: run full tests on the latest GMT development version