-
Notifications
You must be signed in to change notification settings - Fork 234
Description
In PyGMT wrappers, we typically need to validate parameters, for example, to ensure that required parameters are provided and that mutually exclusive parameters are not used together.
Currently, parameter validation is done inconsistently across wrappers. This is further complicated because most parameters are aliases of GMT’s single-letter options, especially after migrating to the new alias system.
- Check short-form parameters in
kwargsdirectly. e.g.,
Lines 114 to 116 in 4c7e356
| if kwargs.get("F") is None: | |
| msg = "Pass a required argument to 'filter_type'." | |
| raise GMTInvalidInput(msg) |
- Pros: Early validation and simple.
- Cons: Doesn’t work with the new alias system.
- Check long-form parameters directly, e.g.,
Lines 118 to 120 in 4c7e356
| if projection is None: | |
| msg = "The projection must be specified." | |
| raise GMTInvalidInput(msg) |
- Pros: Simple and intuitive.
- Cons: Doesn’t catch users passing short-form options like R="...".
- Check both long-form and short-form parameters in
kwargs,e.g.,
pygmt/pygmt/src/grdlandmask.py
Lines 111 to 113 in 4c7e356
| if kwargs.get("I") is None or kwargs.get("R", region) is None: | |
| msg = "Both 'region' and 'spacing' must be specified." | |
| raise GMTInvalidInput(msg) |
Here kwargs.get("R", region) checks both region and R.
Pros:
- Covers both long- and short- forms
- Works before initializing
AliasSystemso early validation
Cons:
- Slightly hard to read and sometimes we may forget to check both
- Check the
aliasdictafter initializingAliasSystem, e.g.,aliasdict.get("r")in PR pygmt.grdsample: Add check and test for exclusive parameters 'toggle' and 'registration' #4183.
Lines 110 to 112 in c614d6f
| if toggle and aliasdict.get("r") is not None: | |
| msg = "Parameters 'toggle' and 'registration' cannot be used together." | |
| raise GMTInvalidInput(msg) |
Pros:
- Handles both long and short forms
- Readale and no need to check the long-form explicitely
Cons
- Must be done after initializing
AliasSystem - Not all parameters are used in the
Aliasystem, which means sometimes we still need to use option 2
I guess option 3 is better?