CDF metadata: alphabetize attribute keys in *_attrs.yaml#3116
Conversation
Sort the nested attribute mappings alphabetically across the CDF metadata config files in imap_processing/cdf/config/. Top-level product/variable order is unchanged; only each entry's attribute list is reordered. Adds a regression test (test_metadata_yaml_order.py) that fails if any nested attribute mapping drifts out of alphabetical order. Closes IMAP-Science-Operations-Center#2892
|
From Claude: To pre-empt review concerns about whether anything beyond key reordering changed: I ran a layered byte-level audit comparing every modified file against
The last check is the strongest — taking the bag of lines in each file before and after, they're identical sets, just reordered. That means no quoting changed, no indentation changed, no whitespace within any line changed, and no value changed. The diff stat (3,298+ / 3,298−, perfectly balanced) corroborates pure reordering. Two small non-reorder changes worth flagging:
|
Removes the `if not isinstance(doc, MappingNode): continue` outer check. Every imap_processing/cdf/config/*_attrs.yaml is a top-level mapping, so this branch was never exercised. Codecov was flagging it as uncovered, pulling the patch under the 90% target.
There was a problem hiding this comment.
I looked through first 20 files and I didn't see any typo mistakes. That's why I skimmed later files. The alphabetic order work looks great!! Thank you!
Regarding excluding imap_default_global_cdf_attrs.yaml, I am indifferent. Since you added test for checking that everyone follow same convention, I want to say go ahead and update that as well.
There was a problem hiding this comment.
Thank you! This is why alphabetic order helps in code tracking. As we can see, it can be hard to trace code at times because everything is in different places!
|
|
||
| assert not unsorted_blocks, ( | ||
| "CDF metadata attribute keys must be in alphabetical order:\n" | ||
| + "\n".join(unsorted_blocks) |
There was a problem hiding this comment.
Thank you for adding this check! This will keep keep this work and convention going forward! I am glad to see you added filename and key that was out of order for user debugging purposes.
- Resolve conflicts in imap_idex_l1a_variable_attrs.yaml: incorporate new elsec_evtpkt/elssec_evtpkt variables and shfine UNITS fix from dev (IMAP-Science-Operations-Center#3111), with keys alphabetized - Resolve conflicts in imap_lo_l1a_variable_attrs.yaml: drop DEPEND_1 from esa_step_coord, azimuth_6, azimuth_60, and spin per dev (IMAP-Science-Operations-Center#3075) - Alphabetize top-level keys in imap_default_global_cdf_attrs.yaml - Alphabetize keys in ultra event_id variable (added by auto-merge)
|
Just pushed a commit that alphabetized Merging now! |
Summary
Closes #2892.
Alphabetizes the nested attribute mappings in
imap_processing/cdf/config/*_attrs.yamlacross all instruments, so attribute keys (e.g.CATDESC,DEPEND_0,FILLVAL,FORMAT,LABLAXIS,UNITS,VALIDMIN,VALIDMAX,VAR_TYPE, ...) appear in a consistent order. Top-level product/variable ordering is unchanged; only each entry's attribute list is sorted. Some instruments were already alphabetized and were left untouched.The YAML diff is purely a reorder: 3,298 insertions and 3,298 deletions, perfectly balanced across 49 files.
yaml.safe_loadof every modified file matchesdev, so no values were changed.A couple of small incidental fixes that came along: missing trailing newlines were added to
imap_mag_global_cdf_attrs.yamlandimap_idex_global_cdf_attrs.yaml.New test
A new regression test,
imap_processing/tests/cdf/test_metadata_yaml_order.py, fails if any nested attribute mapping drifts out of alphabetical order.It lives in its own new file because it's a structural-hygiene check on the YAML files themselves, not a test of
ImapCdfAttributesorcdf.utils. Happy to fold it intotest_imap_cdf_manager.pyif reviewers prefer, but that felt wrong to me.Out of scope / open question
imap_default_global_cdf_attrs.yamlwas left untouched. Its keys (Project,Source_name,Discipline,Mission_group, ...) sit at the top level rather than being nested under a product/variable, and they aren't alphabetical today (likely following ISTP conventional order). Happy to alphabetize that file too if reviewers feel it's in scope for #2892.Test plan
pre-commit run --all-filespassespytest imap_processing/tests/cdf -qpasses (14 passed, including the new regression test)*_attrs.yaml(recursive walk of nested mappings)yaml.safe_loadmismatches vsdev