Conversation
|
It turns out that we had a good chunk of unspecified defaults in the config that are now being heavily caught. |
Documentation build overviewFiles changed
Show files (3) | 3 modified | 0 added | 0 deleted
|
agitter
left a comment
There was a problem hiding this comment.
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>
|
(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.
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.