-
Notifications
You must be signed in to change notification settings - Fork 25
refactor!: config parsing #292
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
Conversation
|
It turns out that we had a good chunk of unspecified defaults in the config that are now being heavily caught. |
annaritz
left a comment
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.
Suggestion: rename raw_config to config_specs. Keep config.py as named for now, but a later refactor might be in order.
@agitter if you can take a look at this and give some comments, that would be great!
Documentation build overviewFiles changed
Show files (3) | 3 modified | 0 added | 0 deleted
|
agitter
left a comment
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.
The additional comments are helpful for me. I don't have any remaining major concerns. I am not sure how to test this beyond the test cases we have.
Neha, do you want to do more review before we merge?
|
I’ll do another review before merging. |
We could also discuss this and the follow up PRs at our Monday meeting if necessary |
Co-authored-by: Neha Talluri <78840540+ntalluri@users.noreply.github.com>
ntalluri
left a comment
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.
@agitter Tristan addressed all of my remaining questions. This is ready to merge, but we should hold off until we look over the other PRs today in case we decide to wait on merging.
|
(Is that to avoid merge conflicts?) |
|
I have a feeling we’ll end up merging it later today, but it would be helpful to first have a conversation about all the PRs that depend on this one. There may be a reason not to merge it yet, though nothing comes to mind right now. |
|
Actually, I'll merge upstream first. (done, pre-commit passed 👍) |
agitter
left a comment
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.
it would be helpful to first have a conversation about all the PRs that depend on this one
That was to understand the dependent changes and how much further restructuring was coming and what new maintenance burden, if any, would be introduced.
Closes #299. I did want to keep this separate from #228 so that way those details didn't get lost, but this does the proper work of using structural parsing on the config, throwing useful errors on missing keys, wrongly-written keys, default paramaters, and with proper input validation and casting.
This doesn't clean up the parsing done in #228, but it lays the ground for cleaner configuration parsing.
I've also made some issues for suggestions for the configuration format #297 #298 #299 #309.
For users, this affects error messages: Now, we get good error messages for missing keys and too many keys, or improperly formatted values.
Note
The breaking change indicator in the PR header (
!) is present because this will cause the config to raise an error if there are too many keys present inside any one of the objects (except for theruns... object). This also removesreconstruction_settings -> run.