-
Notifications
You must be signed in to change notification settings - Fork 6
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
[WIP] Reader for new commondata #1548
Conversation
f8409fd
to
4c24156
Compare
…dtaed metadata.yaml
0a94f2d
to
8d63b67
Compare
I think it is entirely proper to deprecate python 3.8 whenever we get around merging this. |
" raise ValidationError(f\"{path_str} is not a valid path\") from e\n", | ||
" \n", | ||
"@Parser\n", | ||
"def ValidOperation(op_str: str) -> str:\n", |
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.
Would it make sense to use enums, or Literal for better structure 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.
For the OPs list you mean?
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.
Yes indeed. Would make for better error messages.
"\n", | ||
"# Scalar parsers\n", | ||
"@Parser\n", | ||
"def ValidPath(path_str: str) -> Path:\n", |
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.
Do we want things with more checks, like ExistingFilePath? For fun I tried to reach the error using the visible keys on my keyboard and could not.
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'll see when I actually use it but I'm not sure at this stage the path will be testable for existence
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.
Should be fine either way. Don't have to check for everything at this stage but it sure is nice when sensible.
" FK_tables: list\n", | ||
" operation: ValidOperation\n", | ||
" conversion_factor: float = 1.0\n", | ||
" apfelcomb: dict = None\n", |
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.
This should be Optional[dict]. And probably more structured.
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.
Plan is to remove all dictionaries (if that's what you mean with the structure)
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.
Yes, but also that as it is the default does not type check.
…ary files for the reader (no removals or rawdata)
…ary files for the reader (no removals or rawdata)
We used #1526 as a scratchpad during the meeting so I'll leave it be and instead move here whatever is needed.
For now I've added just a jupyter notebook that reads the new commondata into pandas dataframes.
@enocera @Zaharid if you are ok with those df I'll port the rest (
coredata.py
andcore.py
, specs and so) using that as the underlying objects.(why would the test pass here but not in #1500?)