-
Notifications
You must be signed in to change notification settings - Fork 58
Adding fortitude #150
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
base: main
Are you sure you want to change the base?
Adding fortitude #150
Conversation
james-bruten-mo
left a comment
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 Lucy, looks good. Can you revert the groups for other sites than meto, and change the source to be a positional argument (they'll apply to the core change too).
I also wonder whether it's worth storing this file in SimSys_Scripts and reducing the duplication across Apps and Core? For the different lists of directories to run over, you can set an environment variable in rose-stem/templates/runtime_generate_runtime_scripts.cylc and use that as a command line option - maybe this should default to run over everything. I realise the loops are slightly different, but these can probably be combined?
Also, if you fancied some python cpd, then have a look at pathlib and type hinting, though I won't reject if you don't do it!
rose-stem/site/azngarch/groups.cylc
Outdated
| "scripts": [ | ||
| "config_dump_checker", | ||
| "style_checker", | ||
| "fortitude_linter", |
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.
I'd suggest not adding this to sites other than meto as there's no guarantee they have fortitude available
| "use one in rose-stem/app/check_fortitude_linter/file. " | ||
| "Print output, raise error if any changes required." | ||
| ) | ||
| parser.add_argument( |
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.
This should probably be a positional argument as it's required. You can force that by removing the -s argument and changing --source to just source. Then you'll need to remove the -s from the call in the rose-app.conf
PR Summary
Sci/Tech Reviewer: @james-bruten-mo
Code Reviewer: @Pierre-siddall
This adds the fortitude linter to the test suite, and enables one rule to run to demonstrate basic functionality. Subsequent branches will add more rules and testing.
Associated with MetOffice/lfric_core#217
This addition will mean Fortitude will run when the "scripts"/ "developer"/ "all" groups, or "fortitude_linter" group itself is run with the Cylc test suite.
The rule added here can be referenced Fortitude documentation website here: https://fortitude.readthedocs.io/en/stable/rules/trailing-whitespace/
For reference, these are some example outputs and errors from when fortitude runs:
No errors output:
https://cylchub/services/cylc-review/view/lucy.gordon?&suite=lapps_add_gh_no_errors%2Frun10&no_fuzzy_time=0&path=log/job/1/fortitude_linter/01/job.out
One lint error outputs:
https://cylchub/services/cylc-review/view/lucy.gordon?&suite=lapps_add_gh_no_errors%2Frun8&no_fuzzy_time=0&path=log/job/1/fortitude_linter/01/job.err
Non-lint error outputs:
https://cylchub/services/cylc-review/view/lucy.gordon?&suite=lapps_add_gh_no_errors%2Frun9&no_fuzzy_time=0&path=log/job/1/fortitude_linter/01/job.err
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - lapps_add_gh_no_errors/run5
Suite Information
Task Information
✅ succeeded tasks - 1457
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review