-
Notifications
You must be signed in to change notification settings - Fork 9
Make some slots on MassSpectrometryConfiguration and ChromatographyConfiguration required and create a migrator #2465
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
Make some slots on MassSpectrometryConfiguration and ChromatographyConfiguration required and create a migrator #2465
Conversation
|
There are a lot of new invalid tests, I'll let @turbomam decide if we want to keep them all or cull to a couple examples. |
I'll review this migrator later today. Thanks for implementing this! |
...a/migrators/partials/migrator_from_11_7_0_to_11_8_0/migrator_from_11_7_0_to_11_8_0_part_3.py
Outdated
Show resolved
Hide resolved
...a/migrators/partials/migrator_from_11_7_0_to_11_8_0/migrator_from_11_7_0_to_11_8_0_part_4.py
Outdated
Show resolved
Hide resolved
...a/migrators/partials/migrator_from_11_7_0_to_11_8_0/migrator_from_11_7_0_to_11_8_0_part_4.py
Outdated
Show resolved
Hide resolved
...a/migrators/partials/migrator_from_11_7_0_to_11_8_0/migrator_from_11_7_0_to_11_8_0_part_4.py
Outdated
Show resolved
Hide resolved
...a/migrators/partials/migrator_from_11_7_0_to_11_8_0/migrator_from_11_7_0_to_11_8_0_part_4.py
Outdated
Show resolved
Hide resolved
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 left feedback about the part_3
migrator in PR #2461.
I left feedback about the part_4
migrator here in this PR. The migrator looks good to me. I think it is already functionally done; i.e. that it will do what it was designed to do. I left feedback about formatting (consistency of newline inclusion), changing some "AND"s to "OR"s, and a missing word (i.e. "have") in a docstring. I also left a comment about calling dict.get(key)
once up front and then referencing the result as needed.
…migrator_from_11_7_0_to_11_8_0_part_4.py Co-authored-by: eecavanna <[email protected]>
…migrator_from_11_7_0_to_11_8_0_part_4.py Co-authored-by: eecavanna <[email protected]>
…migrator_from_11_7_0_to_11_8_0_part_4.py Co-authored-by: eecavanna <[email protected]>
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.
Thanks, this is good.
I see that some kinds of configurations were added to example files that were supposed to be invalid because they were lacking a configuration
If the RP actions passed (including the examples runner in the makefile) then I guess everything is technically OK, but I am a little confused.
Does my poorly worded concern make sense? I can flesh it out if necessary. The goal is for the example data files to be invalid for one and only one reason... maybe that's what you were working towards?
...a/migrators/partials/migrator_from_11_7_0_to_11_8_0/migrator_from_11_7_0_to_11_8_0_part_3.py
Outdated
Show resolved
Hide resolved
…migrator_from_11_7_0_to_11_8_0_part_3.py Co-authored-by: eecavanna <[email protected]>
…migrator_from_11_7_0_to_11_8_0_part_3.py Co-authored-by: eecavanna <[email protected]>
…into 2456-massspectrometryconfigurations-chromatographyconfigurationslots-required
MERGE AFTER #2461
Closes #2457
Closes #2456