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?
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.