-
Notifications
You must be signed in to change notification settings - Fork 12
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
Initial attempt of setting default constituent value. #226
Initial attempt of setting default constituent value. #226
Conversation
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.
Looks good, but I do have a couple requests and a couple questions.
Updating debug message from PR. Co-authored-by: Jesse Nusbaumer <[email protected]>
…nge to correctly handle ine length.
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.
Just requesting a couple of updates to the new error checks, after which I think this PR will be good to go (at least for me).
Updating not equal comparison to match current code style. Adding line number and file name to error message. Co-authored-by: Jesse Nusbaumer <[email protected]>
Updating not equal comparison to match current code style. Adding line number and file name to error message. Co-authored-by: Jesse Nusbaumer <[email protected]>
Good call on both changes! Updated. |
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.
Looks great now, thanks!
@peverwhee @cacraigucar It looks like I'll need these changes as well in order to properly validate the physics schemes in |
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.
One-ish comment from me, but looks good overall (assuming you also address Cheryl's comments)
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.
looks good! Thanks!
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.
Revisions look good to me
@cacraigucar I updated the error message formatting on this last commit, could you take a look at the implementation and just let me know if it seems reasonable? Just want to make sure the way I'm using the string concatenation is consistent. |
New implementation will attept to find constituent value from file, otherwise will default to value declared in meta files, otherwise 0.