Skip to content
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

Add validation to settings using schema #595

Merged
merged 46 commits into from
Jun 7, 2022
Merged

Add validation to settings using schema #595

merged 46 commits into from
Jun 7, 2022

Conversation

keckler
Copy link
Member

@keckler keckler commented Mar 11, 2022

Description

Add validation to many of the framework settings using the schema parameter and voluptuous. Mainly validating ranges and sizes of user inputs.


Checklist

  • The code is understandable and maintainable to people beyond the author.
  • Tests have been added/updated to verify that the new or changed code works.
  • There is no commented out code in this PR.
  • The commit message follows good practices.
  • All docstrings are still up-to-date with these changes.

If user exposed functionality was added/changed:

  • Documentation added/updated in the doc folder.
  • New or updated dependencies have been added to setup.py.

Copy link
Member

@john-science john-science left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love that you filled out the ranges.

I'm not sure that using "DB" over "database" was really a problem before. But I don't have strong opinions.

I liked that you filled out some missing labels and descriptions.

@keckler
Copy link
Member Author

keckler commented Mar 11, 2022

This is in response to #580

@keckler
Copy link
Member Author

keckler commented Mar 11, 2022

Hm... The test_kineticsParameterAssignment test is failing. I see why. Lemme change.

@john-science john-science linked an issue Mar 11, 2022 that may be closed by this pull request
@jakehader
Copy link
Member

@keckler, the changes look good to me, but there are still some failing unit tests that should be addressed before merging in.

@keckler
Copy link
Member Author

keckler commented May 2, 2022

Yes, I know, a few things still need to be addressed here. I was waiting before for the XML stuff to be removed from the framework, and then I got distracted. This is still on my radar though!

@keckler keckler marked this pull request as draft May 25, 2022 20:26
@keckler keckler marked this pull request as ready for review May 27, 2022 23:00
@keckler
Copy link
Member Author

keckler commented May 27, 2022

Alright, well even though I wanted to figure a way around #669, I didn't. So the schemas look kinda gross sometimes, but whatever.

@keckler
Copy link
Member Author

keckler commented May 30, 2022

@jakehader @john-science
Finally, I think this PR is ready. Sorry for taking so long on it!

@john-science
Copy link
Member

I will merge this tonight: we had a couple of other merges today, and I just want to give some time for all of the downstream repos to run their CI separately.

Thanks!

@john-science john-science merged commit 8f34348 into terrapower:master Jun 7, 2022
@keckler
Copy link
Member Author

keckler commented Oct 11, 2022 via email

scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add validation to case settings where appropriate
3 participants