reflectivity / orsopy

http://www.reflectometry.org/orsopy/
MIT License
0 stars 8 forks source link

Generate schema with pydantic v2 #126

Closed bmaranville closed 9 months ago

bmaranville commented 9 months ago

Using pydantic v2.x, we can use a TypeAdapter to convert stdlib dataclasses to a pydantic model, which we can then use to generate a schema.

This removes the need to use pydantic dataclasses, and therefore we don't need the indirect import of the dataclass decorator from a central location (which was previously monkey-patched when generating the schema)

I ran across an issue with the Column.unit attribute: in the old schema generator, a constraint was manually added to that attribute at schema-generation time which restricted the value to one of ["1/angstrom", "1/nm", "1", "1/s", None], which is ok for the Qz and R columns but which won't work for additional columns like wavelength (see #125)

For the moment I replicated the old behavior, with a note that it needs to be changed (existing tests all pass right now)

bmaranville commented 9 months ago

It looks like the version of jsonschema pulled in to python 3.6 doesn't have a validator for metaschema version 2020-12 https://json-schema.org/draft/2020-12/schema , which is causing tests for that version of python to fail.

andyfaff commented 9 months ago

It seems that Pydantic V2 only supports Python3.7 and above, https://pypi.org/project/pydantic/2.0/#files.

Possible workarounds include:

I have a preference for the latter option but could live with the former. At some point we're going to have to bump Python versions.

andyfaff commented 9 months ago

Otherwise I think this PR is mergeable.

I wonder if we can actually remove the generated yaml and json schemas from the repository? Having them present will lead to churn as they seem to change frequently. Instead we could add a build step in which the schemas are generated. When wheels are generated the schemas will be present in the package.

andyfaff commented 9 months ago

Concerning the column units. I think there is no actual solution in a schema that would allow some units to be checked and others not. I personally would prefer to keep the schema the same as the class hint and to add an additional check to the validation code we use.

So allow any string for units in the schema, and add extra code to _validate_header_data to check the first four columns?

andyfaff commented 9 months ago

Last commit alters the validation function to emit a ORSOSchemaWarning for cp36.

andyfaff commented 9 months ago

All tests pass now (except the coverage aspect), so merging. Thanks everyone.