sciapp / sampledb

Sample and Measurement Metadata Database
https://scientific-it-systems.iffgit.fz-juelich.de/SampleDB/
MIT License
21 stars 11 forks source link

Validation preprocessor #7

Closed MayerBjoern closed 3 years ago

MayerBjoern commented 3 years ago

This pull request is for a preliminary review. The code is tested, but i have not yet created pytest test cases or updated the docs.

Features:

Additional fixes:

FlorianRhiem commented 3 years ago

Thank you for the pull request!

I think they way you integrated the preprocessor should be fine, as no copies are being validated right now, though this needs to be documented to make sure nobody valdidates (and thereby implicitly fixes) a copy of some invalid data. Maybe the modifications done by the preprocessor should be enabled by a parameter (False by default) to avoid accidents like that?

Partial updates via the API can definitely be useful to make life easier, though I think it would be better to have this as an explicit decision by the user, instead of it being based on configuration value. Perhaps by using the PATCH verb instead of POST? PATCH might be more meant for in-place changes instead of creating a new resource as it's done here. If it was done this way instead of changing the established behavior, the configuration variable might not be necessary.

You perform an equality check between the pint quantities. Is there a guarantee that float arithmetic cannot cause a false inequality? If not, a small relative epsilon might be better?

If it's not too much work for you, maybe split this PR into two, one for the quantity changes (and the general preprocessor setup), and one for the partial updates built on top of that one?