-
Notifications
You must be signed in to change notification settings - Fork 229
Add new exception GMTParameterError for invalid parameters #4003
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
cc89cb1
to
a4082e0
Compare
cf33edc
to
e51bb31
Compare
pygmt/exceptions.py
Outdated
if required: | ||
msg = ( | ||
"Required parameter(s) are missing: " | ||
f"{', '.join(repr(par) for par in required)}." | ||
) |
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.
For this case where the parameter(s) is required/compulsory, I'm wondering if we can move towards having the error being thrown natively by Python, instead of writing code like:
if kwargs.get("F") is None:
raise GMTParameterError(required={"filter_type"})
Should we revisit #2896 which mentions PEP692, and also think about using PEP655's typing.Required
qualifier (https://typing.python.org/en/latest/spec/callables.html#required-and-non-required-keys)? Maybe a step towards #262 is to disallow single character arguments for required parameters, so that we can have native Python errors?
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 main issue is that, with the new alias system #4000 implemented, **kwargs
will no longer be needed. We will still keep **kwargs
to support single-letter option flags, but other parameters won't be passed via kwargs
anymore.
947d3f4
to
c2c81d7
Compare
No description provided.