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 writer style option to SettingsWriter pt2 : Propagate to additional methods #932

Merged
merged 12 commits into from
Oct 11, 2022

Conversation

opotowsky
Copy link
Member

@opotowsky opotowsky commented Oct 10, 2022

Description

This propagates the writing style argument to clone and writeInputs (as well as the related CLI for clone).

Sorry about the history. I was working on this branch in parallel to other branches. I'll clean it up in the squash n merge. Edit: working on cleaning it up via an interactive rebase


Checklist

  • This PR has only one purpose or idea.
  • Tests have been added/updated to verify that the new/changed code works.
  • The release notes (location doc/release/0.X.rst) are up-to-date with any bug fixes or new features.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in setup.py.

@opotowsky opotowsky added the enhancement New feature or request label Oct 10, 2022
At the point this is happening and being processed, clone doesn't know
about the "old" filepath, just the destination. Need to finish fixing.
The medium write style requires clone to pass in knowledge about the
"from" location, not just the "to" location, so that the original
settings can be read in. This was mostly done in the previous commit,
but this commit fixes the errored path determination in writeToYamlFile.
Copy link
Member

@keckler keckler left a comment

Choose a reason for hiding this comment

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

Looks good, glad to have this new feature! Thanks for your work on it!

armi/cases/case.py Outdated Show resolved Hide resolved
armi/settings/caseSettings.py Outdated Show resolved Hide resolved
armi/settings/caseSettings.py Outdated Show resolved Hide resolved
armi/cases/suite.py Outdated Show resolved Hide resolved
armi/cases/tests/test_cases.py Show resolved Hide resolved
@opotowsky opotowsky merged commit 06b839e into terrapower:main Oct 11, 2022
@opotowsky opotowsky deleted the add_writer_style_pt2 branch October 11, 2022 14:24
scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
…al methods (terrapower#932)

* Add settingsWriteStyle arg to case and suite cloning CLI/modules

* WIP add unit tests

* Docstring fixes

* Fix for using clone with medium write style

The medium write style requires clone to pass in knowledge about the
"from" location, not just the "to" location, so that the original
settings can be read in. 

* Add a test for clone

* Adding a little more to writeInputs test

* Release notes

* Missed a commit in interactive rebase...this fixes what was missing

* Address reviewer comment: add comments to explain expected test results

* Address reviewer comments: More descriptive docstrings, and moving a line under the if statement it pertains to
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants