Fix problems with tools by mkavulich · Pull Request #145 · ESCOMP/ESMStandardNames

mkavulich · 2026-04-13T22:14:11Z

Description

In the process of trying to add a new tool to check for redundant CF name elements (see #143), I found that our existing tools suffer from a number of issues:

Several tools were not updated appropriately to deal with nested sections (Changes after rename of CCPPStandardNames --> ESMStandardNames #98) so they are not properly checking all the names in the dictionary
- This meant that there were several redundant standard names that had to be removed
A bad merge at some point re-introduced some non-working code from the framework in tools/write_standard_name_table.py; since doctests are not used in this repository, I removed those tests and the un-exercised logic
Changes in Remove version information from schema filename, remove obsolete features #142 that standardized the spelling of the schema file left a lot of stub logic that should be removed

This PR resolves those problems. Also, I took the opportunity to clean up the existing tools:

Make all python scripts pass pylint with a 10/10 score
- Removed all old format strings, replacing with f-strings (Update python scripts to use f-strings #118)
Standardized the format of comments and multi-line strings
Standardized how command-line arguments are named and referenced
Removed many unnecessary comments

And finally, I added the check for duplicate cfname elements that was the start of this whole process.

Issues

Resolves #118, #144

…t to always resolve these problems manually

…ed with a bad merge confict resolution

- Greatly simplify logic for reading schema file and handling exceptions in that process - Remove unnecessary logic comments

…mments and strings

mkavulich

Including some comments to make reviews easier

mkavulich · 2026-05-07T15:14:01Z

    return result

-###############################################################################
-def find_schema_file(schema_root, schema_path=None):


Note: this tool is no longer needed since we no longer have the schema version in the filename (#142)

mkavulich · 2026-05-07T15:15:15Z

 ###############################################################################
    """Read the XML file, <filename>, and return its tree and root"""
-    if os.path.isfile(filename) and os.access(filename, os.R_OK):
-        file_open = (lambda x: open(x, 'r'))


This logic was simplified as a suggestion from pylint

mkavulich · 2026-05-07T15:21:28Z

-#Function to extract standard names from element tree root
-#+++++++++++++++++++++++++++++++++++++++++++++++++++++++++
-
-def get_dict_stdnames(xml_tree_root):


Renamed this function to get_standard_names_as_set to avoid confusion (it returns a set, not a dict)

mkavulich · 2026-05-07T15:24:05Z


 #+++++++++++++++++++++++++++++++++++++++++++++++++++++++++

-############


This logic isn't really changed all that much, just moved into function main_func(); choosing the "Hide whitespace" option will make this bit easier to review.

mkavulich · 2026-05-07T15:26:04Z

@@ -0,0 +1,8 @@
+"""Shared utilities for ESM Standard Names tools."""


This allows us to import these functions directly from tools in the directory above; note the simplified logic in imports at the top of those scripts.

mkavulich · 2026-05-07T15:28:17Z

    #get list of all standard names
    all_std_names = []
-    for name in root.findall('./section/standard_name'):
+    for name in root.findall('.//standard_name'):


The change that started this whole rabbit hole: we need to check for ALL standard_name entries, not just the top-level ones

mkavulich · 2026-05-07T15:29:50Z

+            else:
+                rm_elements = root.findall(f'.//standard_name[@{args.field}="{dup}"]')[1:]
            print(f"{dup}, ({len(rm_elements)} duplicate(s))")
-        if args.overwrite:


I made a judgement call here that we always want to manually resolve duplicate entries. Doing it automatically seems prone to error (e.g. they were intended to be separate entries but had the same standard name due to a typo).

mkavulich · 2026-05-07T15:31:07Z

-def standard_name_to_description(prop_dict, context=None):
+def standard_name_to_description(prop_dict):
 ########################################################################
-    """Translate a standard_name to its default description


These checks were remnants from code originally copied from the framework, and have no function here

mkavulich · 2026-05-07T15:31:35Z

            match = _REAL_SUBST_RE.match(description)
-        # end while
    else:
-        description = ''


More logic leftover from ccpp-framework

mkavulich · 2026-05-07T15:33:16Z

-        file_ok = validate_xml_file(stdname_file, schema_name,
-                                     None, schema_path=schema_root,
-                                     error_on_noxmllint=True)
-    except ValueError as valerr:


This is convoluted logic to catch an error raised by the validate_xml_file function....it's way simpler and better practice to just let the exception be raised and reference the resulting error trace.

mkavulich mentioned this pull request Apr 14, 2026

Add field for CF equivalent name #143

Merged

mkavulich force-pushed the feature/fix_tools branch from f141318 to edaf76a Compare May 4, 2026 21:38

mkavulich added 16 commits May 7, 2026 09:08

Fix tools to check sections recursively for standard names

4d3d2c8

Standardize how standard names file is provided as an argument

43b5cac

Now that duplicate check is actually working, remove duplicates

266e9ef

Add check for duplicate cfnames

93c195f

Remove logic to remove duplicate attributes/elements from XML; we wan…

0835a43

…t to always resolve these problems manually

Working towards getting all tools to pass pylint checks

ef0f899

Remove references to CCPPError; this appears to have been re-introduc…

89c9b63

…ed with a bad merge confict resolution

remove duplicates from Metadata files

651234e

- Continue linting with pylint

8cc5418

- Greatly simplify logic for reading schema file and handling exceptions in that process - Remove unnecessary logic comments

Continue pylint

4903090

Everything pylints 10/10! Set up pylint to run for GitHub CI

641af4f

Remove redundant/unnecessary comments; combine unnecessarily short co…

0e74b03

…mments and strings

Forgot to standardize input arguments to tools/sort_standard_names.py

603cec6

Add pylint for pylint test

c1bbfed

Some final pylint complaints

1ded274

fix failing tests

4e89dac

mkavulich force-pushed the feature/fix_tools branch from edaf76a to 4e89dac Compare May 7, 2026 15:09

mkavulich marked this pull request as ready for review May 7, 2026 15:09

mkavulich requested review from MarekWlasak, cacraigucar, climbfuji, dustinswales, grantfirl, mwaxmonsky, nusbaume, peverwhee, ss421 and svahl991 as code owners May 7, 2026 15:09

mkavulich commented May 7, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix problems with tools#145

Fix problems with tools#145
mkavulich wants to merge 16 commits intoESCOMP:mainfrom
mkavulich:feature/fix_tools

mkavulich commented Apr 13, 2026 •

edited

Loading

Uh oh!

mkavulich left a comment

Uh oh!

mkavulich May 7, 2026

Uh oh!

mkavulich May 7, 2026

Uh oh!

mkavulich May 7, 2026

Uh oh!

mkavulich May 7, 2026

Uh oh!

mkavulich May 7, 2026

Uh oh!

mkavulich May 7, 2026

Uh oh!

mkavulich May 7, 2026

Uh oh!

mkavulich May 7, 2026

Uh oh!

mkavulich May 7, 2026

Uh oh!

mkavulich May 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant


		#+++++++++++++++++++++++++++++++++++++++++++++++++++++++++

		############

		@@ -0,0 +1,8 @@
		"""Shared utilities for ESM Standard Names tools."""

Conversation

mkavulich commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues

Uh oh!

mkavulich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mkavulich commented Apr 13, 2026 •

edited

Loading