Move shellouts#40
Conversation
|
Erica Neininger (@ericaneininger) hopefully this should be ready to go on now, let me know if there's anything else that is needed. |
Erica Neininger (ericaneininger)
left a comment
There was a problem hiding this comment.
Just a couple of typos and copyright dates to fix please.
mocilib/shellout.py
Outdated
| formatted_cmd = list(cmd) | ||
|
|
||
| try: | ||
| print(f"Attempting to execute {formatted_cmd}") |
There was a problem hiding this comment.
There are a couple of print statements in this file. For consistency, please can we stick to if verbose: sys.stdout.write(...) for info or sys.stderr.write(...) for error messages.
If these prints statements are simply debug statements (this one in particular!) for your own testing then the should be removed.
There was a problem hiding this comment.
Good point I forgot that debug message was there
Co-authored-by: Erica Neininger <[email protected]>
Co-authored-by: Erica Neininger <[email protected]>
Co-authored-by: Erica Neininger <[email protected]>
Co-authored-by: Erica Neininger <[email protected]>
Co-authored-by: Erica Neininger <[email protected]>
|
I'm hoping that is fixed now, apologies for not removing those debug messages as agreed. |
Erica Neininger (ericaneininger)
left a comment
There was a problem hiding this comment.
Thanks for the changes. Happy to approve
443e7ce
into
MetOffice:main
PR Summary
Code Reviewer: Erica Neininger (@ericaneininger)
This PR aims to unify shell out commands across MOCI so that they all have the same semantics. The changes involved include adding a separate library to handle shell operations and replacing current shell operations with a newly implemented
exec_subprocessfunction and subsequently also deprecates legacy shell out commands to be redirected to useexec_subprocessinmocilib.closes #12
closes #23
Code Quality Checklist
(Some checks are automatically carried out via the CI pipeline)
readability of the code
Testing
acceptable (eg. kgo changes)
tests, unit tests, etc.)
Test Suite Results - moci - test-mocilib/run2
Suite Information
Task Information
✅ succeeded tasks - 116
Security Considerations
Performance Impact
performance measurements have been conducted
AI Assistance and Attribution
of Generative AI tool name (e.g., Met Office Github Copilot Enterprise,
Github Copilot Personal, ChatGPT GPT-4, etc) and I have followed the
Simulation Systems AI policy
(including attribution labels)
Documentation
confirmed that it builds correctly
Code Review