Add validation to esp-config
#2475
Conversation
| /// Ensure that an integer value falls within the specified range. | ||
| IntegerInRange(Range<i128>), | ||
| /// A custom validation function to run against any supported value type. | ||
| Custom(Box<dyn Fn(&Value) -> Result<(), Error>>), |
There was a problem hiding this comment.
This is more of a general question than a review comment. Can we somehow cross-validate values using Custom? I can't figure out how right now, but I think it would be useful if we could - there are value combinations in esp-wifi that don't make much sense, for example.
There was a problem hiding this comment.
Is this in the scope of esp-config, or should this be managed in the crate itself? The downstream crate will have the config in a const context, it can do the required checks there to ensure that if X is enabled Y also is or isn't etc.
There was a problem hiding this comment.
The downstream crate will have the config in a const context, it can do the required checks there to ensure that if X is enabled Y also is or isn't etc.
That would separate the declaration of the variables (along with their individual validation rules, which are in build.rs) from the cross-validation rules which would be in lib.rs. I'm not sure if that is the ideal state we want to have.
There was a problem hiding this comment.
That's true, but we can actually couple the cross-validations into build.rs too (something I only just remembered), as generate_config returns a hashmap of the key/set values.
I guess what I'm saying is that cross-validaton may come to esp-config in the future, but I think its out of scope of individual config validation, i.e let's not try and abuse Custom to do it.
| "Places the SPI driver in RAM for better performance", | ||
| Value::Bool(false), | ||
| None |
There was a problem hiding this comment.
This is a purely theoretical question, but what does it mean to set a, say, range requirement for a boolean? I guess what I want to say is, wouldn't it be better to describe the validation rule as part of the value type, as Value::XY(default, validation_rule)?
Opening as a draft, as this will need rebasing and additional changes once #2422 and #2246 get merged.
I have not actually taken advantage of the validation in any of the packages depending on
esp-config, not sure if that should be considered in-scope for this or be handled in a subsequent PR. Will take some time to do this.skip-changelogadded as theesp-configpackage does not have aCHANGELOG.md, and there are no user-facing changes here.Closes #2206