AliasSystem: Support adding a suffix to a value and simplify Figure.wiggle#4259
AliasSystem: Support adding a suffix to a value and simplify Figure.wiggle#4259
Conversation
seisman
added
enhancement
pygmt/alias.py
Outdated
| # - If any Alias has a suffix, return a list of values, for repeated GMT options | ||
| # like -Cblue+l -Cred+r | ||
| # - Otherwise, concatenate into a single string for combined modifiers like | ||
| # -BWSen+ttitle+gblue. |
There was a problem hiding this comment.
G=[
Alias(fillpositive, name="fillpositive", suffix="+p"),
Alias(fillnegative, name="fillnegative", suffix="+n"),
],
Taking wiggle's -G option as an example. In the previous version, the values will be concatenated into a single string, so fillpositive="blue", fillnegative="red" will be -Gblue+pred+n, which is invalid.
I've updated the logic of AliasSystem, so that it will return a list of values, rather than concatenating them, when any Alias has a suffix.
There was a problem hiding this comment.
Maybe this a short note about this should be added to the docstring. I'm afraid that this will be missed, e.g. in #4234 (comment) if we forget that suffix has this special treatment.
There was a problem hiding this comment.
What about moving the whole note lines 303-310 to docstrings?
seisman
added
needs review
| # True is converted to an empty string with the optional prefix and suffix. | ||
| if value is True: | ||
| return f"{prefix}" | ||
| return f"{prefix}{suffix}" |
There was a problem hiding this comment.
Would it be confusing if we allowed both prefix and suffix when value is True? E.g. at #4234 (comment), we used prefix, but it could also be suffix.
Or I guess it doesn't matter, and we should just be consistent that every if-branch in this _to_string function handles prefix and suffix.
There was a problem hiding this comment.
Would it be confusing if we allowed both
prefixandsuffixwhen value is True? E.g. at #4234 (comment), we usedprefix, but it could also besuffix.
prefix and suffix cannot coexist. This function does do the check to avoid too much function overhead. It's our responsibility to pass the correct prefix/suffix values.
| Alias(positive_fill, name="positive_fill", suffix="+p"), | ||
| Alias(negative_fill, name="negative_fill", suffix="+n"), |
There was a problem hiding this comment.
Maybe take this opportunity to update the type-hint of postive_fill and negative_fill at L47-48 above?
There was a problem hiding this comment.
Should remove : str from L116 and L118 too?
There was a problem hiding this comment.
Pull request overview
This PR adds suffix support to the Alias class in PyGMT's alias system, enabling cleaner and more maintainable code. The feature is demonstrated by refactoring Figure.wiggle to eliminate the custom _parse_fills helper function.
Changes:
- Added
suffixparameter toAliasclass and_to_stringfunction - Updated
AliasSystemto handle aliases with suffixes by returning them as lists for repeated GMT options - Simplified
Figure.wiggleby removing_parse_fillsfunction and using the new suffix feature - Added type annotations for
positive_fillandnegative_fillparameters inwiggle
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pygmt/alias.py | Added suffix parameter support to _to_string, Alias, and AliasSystem classes with comprehensive documentation and doctests |
| pygmt/src/wiggle.py | Removed custom _parse_fills function, added type annotations for fill parameters, and refactored to use new suffix feature in alias configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
weiji14
left a comment
weiji14
left a comment
There was a problem hiding this comment.
Looks good, just one more minor thing
| Alias(positive_fill, name="positive_fill", suffix="+p"), | ||
| Alias(negative_fill, name="negative_fill", suffix="+n"), |
There was a problem hiding this comment.
Should remove : str from L116 and L118 too?
2 participants
This PR adds
suffixsupport to theAliasclass, which is necessary to support some options, e.g.,wiggle's-Goption has a syntax-Gfill[+n][+p]. Currently, we have to write a custom function to parse the argument. With thesuffixsupport, it can be greatly simplified.