-
Notifications
You must be signed in to change notification settings - Fork 5
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
Refactor Python code #580
Refactor Python code #580
Conversation
I started with splitting the Model into separate tables and config, but didn't get far without hitting the limits of the current design. The problem is, there's no clear design, it's mix of different abstractions. Pandera DataFrames are high-level abstract things, but represent low-level tables (which can come from arrow or geopackage). There's a high-level abstraction of a nodetype in between, but it doesn't represent something low-level. The current Model is a mix of a representation of these nodetypes with a (partial) low-level config. Will make a separate issue about this, which needs a refinement. |
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.
A few, first comments.
I also see that mypy is still unhappy with you.
Will documentation still be part of this PR?
@@ -60,7 +80,7 @@ end | |||
|
|||
os_line_separator() = Sys.iswindows() ? "\r\n" : "\n" | |||
|
|||
function gen_schema(T::DataType, prefix = prefix) | |||
function gen_schema(T::DataType, prefix = prefix; pandera = true) |
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.
Why is pandera
optional?
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.
Because I hacked the remarks field into each type specifically for pandera. But the generation of the Config doesn't need it.
Frankly, this whole file is a hack, but it works. Doing it right requires a new package that actually implements the whole of JSONSchema.
Yeah, not sure if I can fix that nicely, I dynamically add things to a module namespace. Will try though.
Yep, at least about the generation of the config.py files. |
Is it possible to remove the timestamp from the generated python files? It creates merge conflicts: <<<<<<< HEAD
# timestamp: 2023-09-05T14:00:34+00:00
||||||| 1ad1340
# timestamp: 2023-08-21T14:05:19+00:00
=======
# timestamp: 2023-09-04T11:45:02+00:00
>>>>>>> main And with #594 I expect we will run it more often as part of tasks, even if it is not strictly needed. In that case all it would do is update the timestamp. |
…onal and defaults.
Fixes #512
Refactor Python model into split Config & Root submodels