BREAKING: Raise GMTValueError exception for invalid values. Previously raise GMTInvalidInput #3985
Conversation
seisman
force-pushed
the
exception/valueerror
branch
2 times, most recently
from
ddf56de to
efa6715
Compare
seisman
changed the title
seisman
force-pushed
the
exception/valueerror
branch
15 times, most recently
from
340b235 to
8472d86
Compare
seisman
changed the title
seisman
force-pushed
the
exception/valueerror
branch
from
8472d86 to
a590758
Compare
There was a problem hiding this comment.
Thinking if this should be marked as a breaking change, because the exception type changes from GMTInvalidInput to GMTValueError, and code like try: ... except GMTInvalidInput: ... would break. But unsure if users even have code like that which matches on GMTInvalidInput.
Other than that, some minor suggestions/questions below.
pygmt/exceptions.py
Outdated
| """ | ||
|
|
||
|
|
||
| class GMTValueError(GMTError): |
There was a problem hiding this comment.
Should this be subclassed from both GMTError and ValueError?
| value: Any, | ||
| description: str = "value", |
There was a problem hiding this comment.
From your changes, it seems like the value parameter is used in a positional-only way. Do you want to enforce it with https://peps.python.org/pep-0570/#syntax-and-semantics, or still let value be a 'positional_or_keyword_parameters'?
| value: Any, | |
| description: str = "value", | |
| value: Any, | |
| /, | |
| description: str = "value", |
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
| msg = f"Invalid {description}: {value!r}." | ||
| if choices: | ||
| msg += f" Expected one of: {', '.join(repr(c) for c in choices)}." | ||
| if reason: | ||
| msg += f" {reason}" |
| >>> raise GMTValueError("invalid", choices=GridType) | ||
| Traceback (most recent call last): | ||
| ... | ||
| pygmt.exceptions.GMTValueError: Invalid value: 'invalid'. Expected one of: <GridType.CARTESIAN: 0>, <GridType.GEOGRAPHIC: 1>. |
There was a problem hiding this comment.
Error message is slightly different from the previous formatting GridRegistration.GRIDLINE (0) or GridRegistration.PIXEL (1), but hopefully ok.
There was a problem hiding this comment.
<GridType.CARTESIAN: 0> is from the repr() output:
>>> from pygmt.enums import GridType
>>> repr(GridType.CARTESIAN)
<GridType.CARTESIAN: 0>
So I think it's better than previously custom string format.
seisman
changed the title
I've updated the PR title to indicate this is a breaking change. |
weiji14
changed the title
…y raise GMTInvalidInput (#3985) Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Add new exception GMTValueError. The error message will be like
Address #3984 and #3707.