-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
split read/write schema validation #697
split read/write schema validation #697
Conversation
Codecov Report
@@ Coverage Diff @@
## master #697 +/- ##
==========================================
- Coverage 96.08% 96.06% -0.02%
==========================================
Files 92 92
Lines 6330 6350 +20
==========================================
+ Hits 6082 6100 +18
- Misses 248 250 +2
Continue to review full report at Codecov.
|
The sphinx warnings are due to the intersphinx mapping to Scipy is currently broken. |
with open(fname, "w+") as fh: | ||
fh.write(schema.format(shape=shape_s)) | ||
result[1] = fname |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding a testcase for this !
I think there is a way with asdf
to provide a schema without saving an actual file but I am not sure if it works with our WeldxFile
wrapper
Here is an example:
https://github.com/asdf-format/asdf/blob/66abc5e4599db381850dddcb293a90c4a9afd999/asdf/tests/test_schema.py#L196-L204
I will look into if we can apply this somehow in our test cases, it should make a lot of things much easier
but this solution is good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for providing the file-free version of schema validation! This looks very useful. Since WeldxFile respects outer Config
contexts, I think it would be possible to use this pattern. I will try it out quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what might be an issue is that we only look for schemas as files in our current implementation
Lines 221 to 230 in ae2dff5
if custom_schema is not None: | |
_custom_schema_path = pathlib.Path(custom_schema) | |
if not _custom_schema_path.exists(): | |
try: | |
custom_schema = get_schema_path(custom_schema) | |
except ValueError: | |
raise ValueError( | |
f"provided custom_schema {custom_schema} " "does not exist." | |
) | |
asdffile_kwargs["custom_schema"] = custom_schema |
what we might need is another 'fallback' into asdf.schema.load_schema()
in the place where we 'resolve' the schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it is a nice pattern, the custom_schema argument routing (e.g. tuple unpacking) is only possible within the wrapper. Also AsdfFile expects user schemas to be a file or URI: https://github.com/asdf-format/asdf/blob/66abc5e4599db381850dddcb293a90c4a9afd999/asdf/tests/test_schema.py#L899
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we cant use it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this solution 👍
use tmp_path fixture to remove tmp dirs after test run. remove duplicated code.
Changes
WeldxFile now supports two custom_schema sub-arguments. The first argument will be used upon reading, the second upon writing. That way we can use separate schema for both operations. It turned out, that this is useful during schema development.
Related Issues
Closes #690
Checks