Structured error reporting and exhaustive parameter validation #234
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Feb 13, 2023
bpoldrack
previously approved these changes
Feb 13, 2023
Codecov ReportBase: 51.68% // Head: 51.47% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #234 +/- ##
==========================================
- Coverage 51.68% 51.47% -0.21%
==========================================
Files 42 43 +1
Lines 3326 3419 +93
==========================================
+ Hits 1719 1760 +41
- Misses 1607 1659 +52
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
mih
force-pushed
the
validation
branch
4 times, most recently
from
79a05c9 to
a3a06ab
Compare
Even if they are provided by other parts of datalad_next, this helps to keep the `contraints` module more self-contained. All constraints should import outward-facing exceptions (i.e., those raised, and not caught internally, to communicate particular issues).
This is a further improvement of the `constraints` module (ping datalad#115). There is a new base class `ConstraintError` that can be used be `Constraint` implementations to communicate validation errors. it derived from `ValueError` (the former and current choice for raising exceptions on validation errors) to keep `except` clauses in consumer code valid. With `ConstraintErrors` there is a second base class that can be used for associating constraint violations with a particular context. The specialization `CommandParametrizationError` uses this pattern to communicate constraint violations for a full command parameterization at once (for each parameter individually, and for the joint parameter set in addition). An improved `EnsureCommandParameterization` constraint uses these new possibilities to offer more uniform and more detailed information on parameter validations. With this change, a caller can now also decide whether to perform an exhaustive parameter validation, or to fail on first error. The `Constraint` base class has received a new convenience method `raise_for()` that can be used to raise the new `ConstraintError` with minimal effort. Only a message (as before with `ValueError`), and the value that was found to be in violation need to be passed. Thew new approach to exception raising is demo'ed for `AltConstraints`, and `EnsureChoice`. Beyond that, no other constraints have been adjusted yet. Closes datalad#198 Closes datalad#199 (ping datalad#141)
This extends and formalizes the validation of parameter sets (beyond independent validation of individual parameters). The most significant change here is the introduction of `ParameterConstraintContext`, a dataclass representing such a validation context (see docstring). Based on this new class, all other plain-text parameter label identifiers have also been replaced with this class. This makes reporting more uniform, and also makes the `__all__` context label obsolete. A basic test to demo this is included.
This is a new key type to be used in most command parameter validation code.
Instead of a single monolithic `joint_validation()` method, the tests
are now broken into much smaller pieces. They are still incremental
and ordered, hence coercion and checks can be kept in distinct
implementations as needed. This included the ability to re-use
existing/generic `Constraint` implementations for higher-order
checks, and still have their (generic/context-free) error messages make
sense.
There is not need for the `ParamDictator` utility anymore, because all
args come as keyword args, and only relevant args needs to be
anticipated.
Error message generation no longer requires distributed string
interpolation, but messages are more or less constant -- and the
describe what has violated, which context, and how.
From a CLI point-of-view, this changes results in
```
❯ datalad credentials wrong
[ERROR ] 1 command parameter constraint violation
action
is not one of ('query', 'get', 'set', 'remove')
❯ datalad credentials remove
[ERROR ] 1 command parameter constraint violation
action, name (remove-action requirements)
no credential name provided
❯ datalad credentials get
[ERROR ] 1 command parameter constraint violation
action, name, spec (get-action requirements)
no name or credential property specified
```
Add test to show where that matters.
while at it, also improve the messaging to be more comprehensive in case an actual closed interval is to be matched.
This shall be the place to implement any querying that would be needed by consuming APIs for adequate error reporting/rendering.
This is an anticipation of a future implementation change, where mutually independent checks are executed in parallel. In such a case, 'immediately' does not make any sense anymore. Likewise, 'raise-before-joint' is not necessary, because via the `ParameterConstraintContext` specifications we know exactly which validations depend on which parameters, and we can reliably determine which ones can still make sense to run (or not, because an earlier validation step for a parameter has already failed, and we cannot assume that the input assumptions of a later check are valid). This leaves: 'raise-early', 'raise-at-end'
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a further improvement of the
constraintsmodule (ping #115).There is a new base class
ConstraintErrorthat can be used beConstraintimplementations to communicate validation errors. itderived from
ValueError(the former and current choice for raisingexceptions on validation errors) to keep
exceptclauses in consumercode valid.
With
ConstraintErrorsthere is a second base class that can be usedfor associating constraint violations with a particular context. The
specialization
CommandParametrizationErroruses this pattern tocommunicate constraint violations for a full command parameterization
at once (for each parameter individually, and for the joint parameter
set in addition).
An improved
EnsureCommandParameterizationconstraint uses thesenew possibilities to offer more uniform and more detailed information
on parameter validations.
With this change, a caller can now also decide whether to perform an
exhaustive parameter validation, or to fail on first error.
The
Constraintbase class has received a new convenience methodraise_for()that can be used to raise the newConstraintErrorwith minimal effort. Only a message (as before with
ValueError),and the value that was found to be in violation need to be passed.
Thew new approach to exception raising is demo'ed for
AltConstraints,and
EnsureChoice. Beyond that, no other constraints have been adjustedyet.
Closes #198
Closes #199 (ping #141)