Skip to content

Conversation

james-bruten-mo
Copy link
Contributor

@james-bruten-mo james-bruten-mo commented Aug 19, 2025

Description

Summary

Refactor validate_rose_meta.py and move it to SimSys_Scripts. The script will now automatically find metadata sections and apps to validate (ignoring a list of exceptions) rather than only testing based on a hardcoded list. This was done previously and meant that new metadata sections were regularly not tested.

Coordinated merge

This ticket needs to be committed before:
LFRic Apps #954
LFRic Core #4666

Checklist

  • I have performed a self-review of my own changes

@james-bruten-mo james-bruten-mo requested a review from a team as a code owner August 19, 2025 13:30
@james-bruten-mo james-bruten-mo requested review from r-sharp and cameronbateman-mo and removed request for a team August 19, 2025 13:30
@james-bruten-mo james-bruten-mo requested review from jennyhickson and removed request for r-sharp August 21, 2025 08:23
Copy link
Contributor

@cameronbateman-mo cameronbateman-mo left a comment

Choose a reason for hiding this comment

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

Made some suggestions for the return values, I found returning a variable named failures with boolean confusing so suggestions should just return True or False.

Other than that all looks good.

james-bruten-mo and others added 2 commits August 27, 2025 10:23
Co-authored-by: Cameron Bateman <cameron.bateman@metoffice.gov.uk>
@james-bruten-mo
Copy link
Contributor Author

Thanks Cameron, I've committed the bottom suggestion which is a good shout. I've not done the other ones. The reason for storing the state in the variable is it lets us run the test for all apps before raising an error, so we find all the issues immediately rather than fixing one and running again

Copy link
Contributor

@cameronbateman-mo cameronbateman-mo left a comment

Choose a reason for hiding this comment

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

All looks good 👍👌

Copy link
Contributor

@jennyhickson jennyhickson left a comment

Choose a reason for hiding this comment

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

Generally good - a few odds and sods :)

Copy link
Contributor Author

@james-bruten-mo james-bruten-mo left a comment

Choose a reason for hiding this comment

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

Thanks Jenny, changes made

Copy link
Contributor

@yaswant yaswant left a comment

Choose a reason for hiding this comment

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

Since this is a completely new addition, please consider type hints.

james-bruten-mo and others added 2 commits August 29, 2025 08:23
Co-authored-by: Yaswant Pradhan <2984440+yaswant@users.noreply.github.com>
Co-authored-by: Yaswant Pradhan <2984440+yaswant@users.noreply.github.com>
@james-bruten-mo
Copy link
Contributor Author

Given I've adapted this from an existing script that was in the Apps repo (and it's pretty straightforward) I'll leave off adding the type hinting for now.

Copy link
Contributor

@jennyhickson jennyhickson left a comment

Choose a reason for hiding this comment

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

Thanks for sorting. Looks good to me.

@jennyhickson jennyhickson merged commit 1f3e980 into MetOffice:main Aug 29, 2025
5 checks passed
@james-bruten-mo james-bruten-mo deleted the validate_rose_meta branch September 14, 2025 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants