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

Use longname placeholders in the docstrings for common options #1932

Merged
merged 48 commits into from
Sep 19, 2022

Conversation

seisman
Copy link
Member

@seisman seisman commented May 25, 2022

Description of proposed changes

For "common" options, we have the option description defined in the file pygmt/helpers/decorators.py. Then in the module wrappers, we can use placeholders like {R} in the docstrings, which are automatically replaced by the long docstrings.

Currently, we use single-letter flags as the placeholders (e.g., {R}, {I}) for most common options, although there are a few exceptions (e.g., {CPT}, {XY}). It works well but sometimes may cause confusion, especially for non-common options like -G and -I.

For example, sometimes the "common" option -G is aliased to fill, but the docstring for placeholder {G} indicates it's aliased to color (e.g., #1617). I also remember that someone once incorrectly used the placeholder {I} in the docstring, but the -I option for the module has very different meanings (for example, -I means inquiry in some modules).

I find that using long names for these placeholders can avoid these problems and long names are also more readable than short names.


In this PR, I rename the placeholder {I} to {spacing} to show what the changes look like, but I plan to replace all short-name placeholders with long names (e.g., {R} => {region}).

What do you think about the changes?

References:

@seisman seisman added the question Further information is requested label May 25, 2022
@maxrjones
Copy link
Member

I agree this is an improvement. In addition to your comments, this also offers a slight benefit to users of IDEs like vscode which do not process the f-strings when displaying the documentation.

@seisman seisman added maintenance Boring but important stuff for the core devs and removed question Further information is requested labels Jun 1, 2022
@seisman seisman added this to the 0.7.0 milestone Jun 1, 2022
@seisman seisman requested a review from a team June 14, 2022 02:27
@seisman seisman modified the milestones: 0.7.0, 0.8.0 Jun 23, 2022
pygmt/src/ternary.py Outdated Show resolved Hide resolved
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 @seisman! Was a little concerned if it's premature to find and replace the not-so-common-options like {I| -> {spacing} since we're not 100% confident that the longnames for these are considered final for GMT. I.e. that someone won't suddenly decide that I should be called interval instead of spacing. But should be ok since we can always change them again 🙂 Certainly not a problem for the very-common-options listed in https://docs.generic-mapping-tools.org/6.4/devdocs/long_options.html#common-options such as B/R/J/U/V/X/Y and so on.

Would be good to have a second review (since this is a big PR), but I did scan through each change one by one and they seem ok.

@weiji14 weiji14 added the final review call This PR requires final review and approval from a second reviewer label Sep 16, 2022
@seisman
Copy link
Member Author

seisman commented Sep 17, 2022

@maxrjones
Copy link
Member

Ping @maxrjones @willschlitzer @michaelgrund @yvonnefroehlich

I can take a look later today or tomorrow.

Copy link
Member

@yvonnefroehlich yvonnefroehlich left a comment

Choose a reason for hiding this comment

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

So far, all replacements appear correct to me.
The fill-color inconsistency (pygmt.Figure.histogram and pygmt.Figure.ternary) seems to be separately discussed in issue #1617.

Comment on lines 85 to 92
Arrays of x and y coordinates and values z of the data points.

{I}
{spacing}

{R}
{region}

search_radius : str
Sets the search radius that determines which data points are considered
Copy link
Member

Choose a reason for hiding this comment

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

In a few functions and methods, blank lines are used between the different parameters. Currently, it appears unclear to me when and why this is done.

Copy link
Member Author

Choose a reason for hiding this comment

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

In a few functions and methods, blank lines are used between the different parameters. Currently, it appears unclear to me when and why this is done.

These blank lines are not necessary. They're there because these docstrings were written by different authors at different time.

@seisman seisman merged commit 9936c9f into main Sep 19, 2022