Repository navigation
feat: add validation of ranged and stepped numerical inputs - #79
Conversation
jacanchaplais
left a comment
There was a problem hiding this comment.
Wow @jpbrodrick89, this looks amazing, thank you! I've added some comments, a nit, and a suggestion for some cleaner data structuring, hope that's alright. :)
As for the slider, would this respond well to logarithmically scaled data? I also get a bit wigged out with sliders if the resolution needs to be precise, and I can't hit the number I want haha. Let me know what you think! :)
|
Also @jpbrodrick89 I like your idea of adding a Tesseract with min / max values. You can use Streamlit's + Typer's testing frameworks as used in |
|
Thanks for the quick review @jacanchaplais , it seems that UI functionality can't really be tested with |
What about iteratively calling the
As in the step doesn't work when you manually test in the web UI? Sounds like the feature is just broken, then? If so, we should either investigate why or remove that feature, since we don't want to introduce non-operational code. Testing-wise, you could probably also use the |
|
I mean step correctly steps up in the correct increments but it doesn't perform validation if you manually input incorrect values. I'll try the increment/decrement methods you recommend though to toughen up the testing. |
|
Unfortunately |
There was a problem hiding this comment.
Sorry for the slow reply @jpbrodrick89! LGTM, thanks :)
Relevant issue or PR
Description of changes
This PR parses the pydantic keys
minimum,maximumandmultipleOfand applies them as constraints innumber_inputthroughmin_value,max_valueandstep.Testing done
Added some relevant keys to mock schemas. I also reformatted them to make them more maintainable and human-readable.
I have not yet done manual testing.
@jacanchaplais I'd appreciate your input on how else we can test this, should we have an example (mock) Tesseract that uses min and max values?
I'd also like to default to using a slider (w/ synced number input if possible) when min and max is provided, but thought I'd leave that for a future PR. Do you have any thoughts on whether that's a good idea?