Check mandatory and unexpected arguments before creating an instance.#219
Check mandatory and unexpected arguments before creating an instance.#219YooSunYoung wants to merge 5 commits intorefactor-3from
Conversation
|
I tried implementing just fallback-ingestion simply with the file, but then the mandatory fields like 'owner' will be ambiguous... So in this PR, I tried using fallback schema instead, that ensures all mandatory fields are retrieved/constructed. And we should probably have integration test/unit test with In the current version, we can't have the keyword of How does this approach sound to you?? |
|
I thought about having a default fallback schema used by default but then it might not work with other facilities files... So I added it as a configuration instead. |
| @@ -258,6 +276,9 @@ def main() -> None: | |||
| schema_id=metadata_schema.id, | |||
| logger=logger, | |||
| ) | |||
| # Merge fallback variable map and the selected schema variable map. | |||
| # Order is important so that the selected schema variable configuration is preferred. | |||
| variable_map = {**fallback_variable_map, **variable_map} | |||
There was a problem hiding this comment.
I would change the logic so the fallback schema is used only if computing variable_map fails.
What if a field is not defined on purpose in the schema?
With this logic, it would get assigned a default value which might not be the intention of the data curator
There was a problem hiding this comment.
Variable map doesn't always end up in the metadata unless it's used in the metadata definition itself.
So having default values in the variable map should be fine I think.
src/scicat_offline_ingestor.py
Outdated
| # Merge fallback schema definitions and the selected schema definitinos. | ||
| # Order is important so that the selected schema schema configuration is preferred. | ||
| fallback_schema_definitions = ( | ||
| {} if fallback_schema is None else fallback_schema.schema | ||
| ) | ||
| schema_definitions = {**fallback_schema_definitions, **metadata_schema.schema} |
There was a problem hiding this comment.
Same comment as before!!!
Please use the fallback only if computing metadata_schema results in an error
There was a problem hiding this comment.
My idea was that the fallback schema definitions will only have the minimum set of definitions that you must have in the metadata schema.
But I guess it's hard to prevent from abusing the fallback schema or/and confuse the users who write metadata schema file...
2 participants
Fixes #214
This fix comes from @nitrosx 's commnet from the chat.
TODO:
I have to work on something else for next couple days but I'll come back to it...
Or anyone can pick this up and continue working on them if needed : D...