Skip to content

BREAKING: Raise GMTValueError exception for invalid values. Previously raise GMTInvalidInput - #3985

Merged
seisman merged 15 commits into
mainfrom
exception/valueerror
Jul 9, 2025
Merged

BREAKING: Raise GMTValueError exception for invalid values. Previously raise GMTInvalidInput#3985
seisman merged 15 commits into
mainfrom
exception/valueerror

Conversation

@seisman

@seismanseisman commented Jun 20, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Add new exception GMTValueError. The error message will be like

Invalid value: 'xxx'. Expected one of: "a", "b", 0, 1. Detailed reasons explaining why it's invalid.

Address #3984 and #3707.

@seismanseisman added the enhancement Improving an existing feature label Jun 20, 2025
@seismanseisman mentioned this pull request Jun 20, 2025
Closed
@seisman
seismanforce-pushed the exception/valueerror branch 2 times, most recently from ddf56de to efa6715CompareJune 21, 2025 04:15
@seismanseisman changed the title POC: Add custom exception GMTValueErrorPOC: Add new exception GMTValueError for invalid valuesJun 21, 2025
@seisman
seismanforce-pushed the exception/valueerror branch 15 times, most recently from 340b235 to 8472d86CompareJune 23, 2025 05:01
@seisman
seisman marked this pull request as ready for review June 23, 2025 05:31
@seismanseisman changed the title POC: Add new exception GMTValueError for invalid valuesAdd new exception GMTValueError for invalid valuesJun 23, 2025
@seismanseisman added this to the 0.17.0 milestone Jun 23, 2025
@seisman
seismanforce-pushed the exception/valueerror branch from 8472d86 to a590758CompareJune 24, 2025 02:27
@seismanseisman added the needs review This PR has higher priority and needs review. label Jun 24, 2025
@seisman
seisman requested a review from a teamJuly 5, 2025 05:35
weiji14
weiji14 reviewed Jul 6, 2025
View reviewed changes

@weiji14weiji14 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment threadpygmt/exceptions.py Outdated
"""


class GMTValueError(GMTError):

@weiji14weiji14Jul 6, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be subclassed from both GMTError and ValueError?

@seismanseismanJul 9, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done in 16c15c9.

Comment threadpygmt/exceptions.py
Comment on lines +100 to +101
value: Any,
description: str = "value",

@weiji14weiji14Jul 6, 2025•
edited by seisman
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Suggested change
value: Any,
description: str="value",
value: Any,
/,
description: str="value",

Comment threadpygmt/exceptions.py Outdated
Comment threadpygmt/xarray/accessor.py
@seismanseisman mentioned this pull request Jul 7, 2025
Merged
weiji14
weiji14 reviewed Jul 7, 2025
View reviewed changes
Comment threadpygmt/exceptions.py
Comment on lines +112 to +116
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}"

@weiji14weiji14Jul 7, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These lines would probably be a good use of t-strings (Template strings, PEP 750), but it is for Python 3.14+

Comment threadpygmt/exceptions.py
>>> 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>.

@weiji14weiji14Jul 7, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Error message is slightly different from the previous formatting GridRegistration.GRIDLINE (0) or GridRegistration.PIXEL (1), but hopefully ok.

@seismanseismanJul 9, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

@seismanseisman changed the title Add new exception GMTValueError for invalid valuesBREADKING: Raise GMTValueError exception for invalid values. Previously raise GMTInvalidInputJul 9, 2025
@seisman

seisman commented Jul 9, 2025

Copy link
Copy Markdown
MemberAuthor

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.

I've updated the PR title to indicate this is a breaking change.

@weiji14weiji14 changed the title BREADKING: Raise GMTValueError exception for invalid values. Previously raise GMTInvalidInputBREAKING: Raise GMTValueError exception for invalid values. Previously raise GMTInvalidInputJul 9, 2025
weiji14
weiji14 approved these changes Jul 9, 2025
View reviewed changes

@weiji14weiji14 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've updated the PR title to indicate this is a breaking change.

Breaking, not breadking 😆

@seismanseisman removed the needs review This PR has higher priority and needs review. label Jul 9, 2025
@seisman
seisman merged commit 4586879 into mainJul 9, 2025
@seisman
seisman deleted the exception/valueerror branch July 9, 2025 22:23
This was referenced Jul 10, 2025
Open
Merged
Merged
seisman added a commit that referenced this pull request Jul 16, 2025
@seisman@weiji14
BREAKING: Raise GMTValueError exception for invalid values. Previousl…
61410c8
…y raise GMTInvalidInput (#3985)
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@weiji14weiji14weiji14 approved these changes

Assignees

No one assigned

Labels

enhancementImproving an existing feature

Projects

None yet

Milestone

0.17.0

Development

Successfully merging this pull request may close these issues.

2 participants

@seisman@weiji14