-
Notifications
You must be signed in to change notification settings - Fork 73
Allow XML to have multiple archetype
elements
#1874
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
Conversation
Pull Request Test Coverage Report for Build 16057951560Details
💛 - Coveralls |
Downstream Build Status Report - 43e32f8 - 2025-07-03 18:29:02 +0000Build
|
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 for making this addition @gonuke. I can see how this can add flexibility for a user to make more modular files. I have a few comments and suggestions.
clean_outs() | ||
# Cyclus simulation input for recipe including | ||
sim_input = os.path.join(INPUT, "include_source_w_archetype.xml") | ||
holdsrtn = [1] # needed because nose does not send() to test generator |
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.
Do we still need this if we don't use nose? Maybe this is outside the scope of this PR and should be a new issue.
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.
great question! I'm not sure where else things like this may exist, so it might be worth a new issue to clean these up
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.
Sounds good. I'll make a new issue for that then.
Improved documentation and comments Co-authored-by: Amanda Bachmann <abachmann@anl.gov> Signed-off-by: Paul Wilson <paul.wilson@wisc.edu>
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 for making these changes @gonuke. I'm approving and will merge once I see the Cyclus tests passing.
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.
Had to go fast to sneak in before Amanda could merge, but this looks good to me as well. Excited to try this out sometime!
I think @trysarahtops will be eager to try this out to make her input files simpler! |
All the tests passed. Merging now. |
The previous XML schema only allowed a single
archetype
block to exist. This change allows multiplearchetype
blocks, as long as there is at least one.A motivating use case for this is to facilitate including XML snippets that contain long lists of facility prototypes that may be useful in many simulations. Without this change, the top level input file must include all the archetype specs for the archetypes that are used in those facilities. With this change, the archetype specs that are used in that included file can be included there to make sure they are available.
Fortunately, this required no code changes and only a change in our XML schema. We already processed archetypes in a way that didn't care how many blocks there were, nor cared if there were duplicates of the same archetype.
A test has been added that demonstrates how to use this kind of inclusion, which requires the
xpointer
metadata in the include line.