-
Notifications
You must be signed in to change notification settings - Fork 228
Figure.coast/pygmt.select/pygmt.grdlandmask: Use long names ("crude"/"low"/"intermediate"/"high"/"full") for the 'resolution' parameter #3013
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
base: main
Are you sure you want to change the base?
Conversation
e4ea5a1
to
d9af3e0
Compare
d9af3e0
to
88be372
Compare
b493d81
to
6840b39
Compare
…ll") for the 'resolution' parameter
6840b39
to
5fac3ae
Compare
fdf817e
to
5e1ebe8
Compare
CodSpeed Performance ReportMerging #3013 will not alter performanceComparing Summary
|
Ping @GenericMappingTools/pygmt-maintainers for thoughts on the proposed changes towards more Pythonic arguments. |
Actually, this PR is not needed if PR #3238 is implemented. |
def _parse_coastline_resolution( | ||
resolution: Literal["auto", "full", "high", "intermediate", "low", "crude", None], | ||
) -> Literal["a", "f", "h", "i", "l", "c", None]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on making a StrEnum out of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the benefits? This private function will likely be removed after #3239 is implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kwargs["D"] = kwargs.get("D", _parse_coastline_resolution(resolution)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The simplest form of this line is:
kwargs["D"] = _parse_coastline_resolution(resolution)
but to support short-form parameters like D="f"
, we have to write the code like:
kwargs["D"] = kwargs.get("D", _parse_coastline_resolution(resolution))
The D="resolution"
line is also removed from the use_alias
decorator. Otherwise, we have to write codes like below:
kwargs["D"] = _parse_coastline_resolution(kwgars.get("D"))
It's fine, but then ruff will complain that the resolution
parameter is unused.
@@ -20,7 +23,6 @@ | |||
A="area_thresh", | |||
B="frame", | |||
C="lakes", | |||
D="resolution", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After removing D="resolution"
, this alias won't be shown in the alias list. Need to wait for #3945.
This PR updates the
resolution
parameter forFigure.coast
/pygmt.select
/pygmt.grdlandmask
to use long names.