Skip to content

Resolve issues: #24, #64, #82, #118, other fixes#148

Open
mkavulich wants to merge 10 commits into
ESCOMP:mainfrom
mkavulich:feature/resolve_issues
Open

Resolve issues: #24, #64, #82, #118, other fixes#148
mkavulich wants to merge 10 commits into
ESCOMP:mainfrom
mkavulich:feature/resolve_issues

Conversation

@mkavulich

@mkavulich mkavulich commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

Description

Resolving several open issues and items noted on previous PRs:

Issues

Resolves #24, #64, #82, #118

@mkavulich mkavulich changed the title Resolve issues: #24, #64, #118, other fixes Resolve issues: #24, #64, #82, #118, other fixes Jun 8, 2026
@mkavulich mkavulich changed the title Resolve issues: #24, #64, #82, #118, other fixes Resolve issues: #24, #64, #118, other fixes Jun 8, 2026
@mkavulich mkavulich changed the title Resolve issues: #24, #64, #118, other fixes Resolve issues: #24, #64, #82, #118, other fixes Jun 8, 2026
@mkavulich mkavulich marked this pull request as draft June 8, 2026 19:11

@climbfuji climbfuji left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I know this is just a draft, but I am happy with the changes.

steps:
- name: Checkout repository
uses: actions/checkout@v4
uses: actions/checkout@v6

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Look how up to date we are!

@mkavulich mkavulich marked this pull request as ready for review June 8, 2026 20:07
@mkavulich

Copy link
Copy Markdown
Collaborator Author

Thanks @climbfuji, I had forgotten a couple things in my original PR so I marked it as a draft while resolving those...the only differences between the original and the latest (ready for review) version is removing Mike W from the codeowners and changing all mentions of "sea level" to "mean sea level" (#82).

@mkavulich

Copy link
Copy Markdown
Collaborator Author

@climbfuji Could you give this an official approval?

@nusbaume @svahl991 I'd appreciate a review when you get a chance, thanks!

@nusbaume nusbaume left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the ping @mkavulich! I have two optional requests, but otherwise this PR looks good to me.

Comment thread tools/lib/xml_tools.py
if not os.access(schema_file, os.R_OK):
emsg = "validate_xml_file: Cannot open schema, '{}'"
raise ValueError(emsg.format(schema_file))
raise FileNotFoundError(f"validate_xml_file: Cannot find schema file {schema_file}")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I might put quotes around {schema_file} here in case a blank string was ever passed in by mistake:

Suggested change
raise FileNotFoundError(f"validate_xml_file: Cannot find schema file {schema_file}")
raise FileNotFoundError(f"validate_xml_file: Cannot find schema file '{schema_file}' ")

Comment thread CODEOWNERS
# These owners will be the default owners for everything in the repo.

* @cacraigucar @climbfuji @dustinswales @grantfirl @mattldawson @mkavulich @mwaxmonsky @nusbaume @peverwhee @MarekWlasak @svahl991 @ss421
* @cacraigucar @climbfuji @dustinswales @grantfirl @mattldawson @mkavulich @nusbaume @peverwhee @MarekWlasak @svahl991 @ss421

@nusbaume nusbaume Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would remove @mattldawson as well, as I don't believe he is with NCAR anymore. I can look for an alternative NCAR chemistry representative in case we need one for future chemistry-related name changes/additions (which I can always add to the CODEOWNERS file in a later PR).

@svahl991 svahl991 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants