-
Notifications
You must be signed in to change notification settings - Fork 6
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
Revamp parameter validation handling #419
Comments
I'm not sure about this:
The alternative would be to validate only when
Because, I think this can be rather expensive. And it's not exactly the command that knows whether it's expensive as it may depend on the dataset. Furthermore, once you run the validation you likely will also need to adjust the form itself. Because there may drop downs for valid input choices that would need to change based on the value of another parameter. So, the paradigm of validating on every change one may turn into form regeneration on every change, which seems even more expensive. Everything else makes sense to me. |
Overall this makes sense to me, in particular 1) the full reevaluation on change to any parameter, and 2) invalid marker with a particular reason. Reminds a bit of reactive frameworks in javascript. In case the full reevaluation is too expensive, would it be possible to have some split between conditional dependency validations and parameter-specific validations, so that independent parameters can be validated on change (e.g. match a regex or should be a number) and ones with conditional dependencies can validate on form submit? |
Thanks for the feedback. I think it is too early to talk about optimizations. Import is the realization that any edit of any value can invalidate any previous validation run. Therefore the system most be able to handle that, and it cannot optimize at the level of individual parameters (which is what it is doing now, and what is wrong). I believe there are plenty of possibilities to make things fast and also be clever about what to revalidate. The key to this ability is the |
The current implementation is written based on the assumption that there is one validator for each parameter, and that validation can be performed individually and independently for each parameter.
However, we know that this is insufficient. Some parameters have conditional dependencies on others (ie. some mode only being applicable to a particular action, etc.)
datalad-next
no bring high-order checks and structured reporting datalad/datalad-next#234This makes it possible to perform such validations, but it is also calling for a rethinking here.
For example, when setting a parameter value in a form field in gooey, only the validator for this individual parameter runs. This is insufficient for concluding that a value of valid -- for the reasons given above.
I believe what should be done is -- on any change -- run the full validator for the entire command parameterization. On error, inspect the structured report for errors connected to the individual parameter that is being modified, and reject the modification if there is anything.
This would require the validator to be associated -- not with a single parameter (like it is now) -- with the entire paramemter form.
Moreover, it may actually lead to a clunky UX when forcefully rejecting parameter changes (the could be complex dependencies that would require awkward procedures to sort them out in a linear fashion). Instead, we might wont to simple mark a parameter as invalid (but expect the edit). based on this mark, we could render the form in a way that the violation is obvious to a user and the reasons for them inspectable.
Taken together:
If this makes sense, it will require a substantial change in the implemented logic.
The text was updated successfully, but these errors were encountered: