Lfric macros jules meta#230
Open
James Bruten (james-bruten-mo) wants to merge 6 commits intoMetOffice:mainfrom
Open
Lfric macros jules meta#230James Bruten (james-bruten-mo) wants to merge 6 commits intoMetOffice:mainfrom
James Bruten (james-bruten-mo) wants to merge 6 commits intoMetOffice:mainfrom
Conversation
28 tasks
Pierre Siddall (Pierre-siddall)
requested changes
May 5, 2026
Contributor
Pierre Siddall (Pierre-siddall)
left a comment
There was a problem hiding this comment.
Thanks James Bruten (@james-bruten-mo), semantically this looks really good, I just have a couple of trivial edits with arguments to help clarify information that is being passed via a function or the command line. Let me know if there's anything I can help clarify.
| # Use /tmp for Core and Jules as these are not required for testing | ||
| applymacros = ApplyMacros("vn0.0_t001", None, None, TEST_APPS_DIR, Path("/tmp"), True) | ||
| applymacros = ApplyMacros( | ||
| "vn0.0_t001", None, None, TEST_APPS_DIR, Path("/tmp"), Path("/tmp"), True |
Contributor
There was a problem hiding this comment.
I reckon it would be good to differentiate between the parameters here as it makes it clearer what data is being operated on when the function is called.
Suggested change
| "vn0.0_t001", None, None, TEST_APPS_DIR, Path("/tmp"), Path("/tmp"), True | |
| "vn0.0_t001", None, None, apps=TEST_APPS_DIR, core=Path("/tmp"), jules=Path("/tmp"), testing=True |
| ) | ||
| parser.add_argument( | ||
| "-j", | ||
| "--jules", |
Contributor
There was a problem hiding this comment.
It could be easy to get a bit confused with apps and core being references to paths. So adding version here may help users of the script know what is expected in the argument.
Suggested change
| "--jules", | |
| "--jules_version", |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary
Sci/Tech Reviewer: Pierre Siddall (@Pierre-siddall)
Code Reviewer: Sam Clarke-Green (@t00sa)
This makes changes to the apply_macros and release_new_version scripts to account for lfric-jules metadata now being in the Jules repository.
Code Quality Checklist
Testing
Security Considerations
AI Assistance and Attribution
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review