Make loading settings robust to subscripted generics.#269
Make loading settings robust to subscripted generics.#269julianstirling wants to merge 10 commits intomainfrom
Conversation
|
Thanks for this. I'd pretty well implemented the tuple setting bit of this, but wasn't checking the logs in the test so missed the error. For testing it would be really good to raise these errors rather than just log them - I might add in a switch somewhere to turn logging errors into hard errors, I think that would be very helpful particularly in testing. |
rwb27
left a comment
rwb27
left a comment
There was a problem hiding this comment.
This adds a very useful test case. There may well be other places where it would be useful to add generic types.
rwb27
dismissed
their stale review
I'm happy with the test, but Julian is working on a fix so I'm going to leave this for now and re-approve once that's pushed.
Rather than separately load a model, then set the property from a model (which requires some checks that are a bit fragile), I've refactored to add a `validate` method to `PropertyInfo`. This creates and then (if needed) unpacks a model in one function, removing the need to check the model is the correct model. I've added a few extra tests to make sure `validate` works for the cases I missed when releasing v0.0.15. I think the logic in `validate` is much simpler and more robust than the isinstance checks that caused the problem.
Just to make sure, I'm checking a generic model and a BaseModel too.
rwb27
changed the title
rwb27
left a comment
rwb27
left a comment
There was a problem hiding this comment.
I was about to approve this and ask @julianstirling for his blessing but I'm now not so sure. I think we are swallowing too many errors in load_settings. I can live with consigning ValidationError and KeyError to the log, but TypeError indicates something is broken, and should not be swallowed. That requires doing something different if we load an non-dictionary object from JSON.
|
I'm realising that adding |
Swap the order of the two cases for clarity. Co-authored-by: Julian Stirling <julian@julianstirling.co.uk>
As discussed in the PR thread, this now hard errors if the settings file is not a valid dictionary, or if TypeErrors occur when loading settings. The latter probably indicates LabThings or Thing code is broken, it should not result from an incorrect settings file. I also fixed a line that was too long.
I've added an extra try: block enclosing just the part that loads the JSON file. I think this neatly separates errors loading the file from errors applying the settings it contains. I have reverted to the old behaviour, of warning for corrupt files and overwriting them. We can add options to make these hard errors in the future.
|
I added a separate It's also much less likely to swallow errors we weren't intending to suppress, which should reduce the likelihood of summarily overwriting the settings file. |
|
@julianstirling I think this is ready now and I'll approve it. I will leave it for you to merge, as I feel you probably need to approve it as well. |
rwb27
left a comment
rwb27
left a comment
There was a problem hiding this comment.
I'm happy with this, bar a docstring that I've just pushed an update to.
I'm approving it but will let @julianstirling merge so someone else has seen my changes.
The nested exception logic was getting confusing I worried the cognative complexity and the length of the function was too high. There is no reason to be in disable_saving_settings mode when loading the file. As such all of the file loading can be done before this starts. Moving it to it's own function provides simplifies it significantly.
julianstirling
force-pushed
the
generic-type-settings
branch
from
044d363 to
580bdab
Compare
|
Right this MR has got pretty bike sheddy. Sorry about that, also ambiguous whose branch it is. I was worried that the second try block nested in the first made the function excessively complicated and only sat within that overall try block because we needed to do the If loading the data happens is fully handled separately from setting the settings. Then there is no reason for it to happen within this block. Moving it to its own private method cleanly separates the file handling logic from the validation and setting* logic. *Setting the verb not setting the noun. This is a confusing sentence to write/read. |
|
@rwb27 I added an extra test. Now this passes. I'm happy, but I'll ping pong it back to you for merge as I committed. |
2 participants
PropertyInfo.model_to_valueusedisinstancewithvalue_typeas the second argument, which doesn't work for subscripted generics. This is the bug that yankedv0.0.15. Subscripted generics may sound esoteric, buttuple[int, int]is a subscripted generic, so it's pretty standard.I've refactored, so we no longer need
model_to_value. Instead,PropertyInfoexposes avalidate()method, which provides the functionality ofmodel_validateusing the property's model. This is easier to type, much more robust, and (I think) rather clearer. As a nice bonus, it accepts models as input, so it replacesmodel_to_value.There are only two cases to consider:
BaseModelsubclass (including subscripted generics). In this case, we can simply returnvalue_type.model_validate()and know it's correctly typed.BaseModel, so we wrap it in aRootModeland use that wrapper (model) to validate. We then need to return itsrootfield, not the whole model instance.I've added test cases for a bunch of different types, but I think the new logic is solid. For both cases, we use exactly one call to
model_validateso we're always sure we have the right type at the end.