Skip to content

Reject whitespace in CF compression level names - #676

Open
nawaaaaaAaar wants to merge 2 commits into
xarray-contrib:mainfrom
nawaaaaaAaar:fix/compression-whitespace-level-names
Open

nawaaaaaAaar wants to merge 2 commits into
xarray-contrib:mainfrom
nawaaaaaAaar:fix/compression-whitespace-level-names

Conversation

@nawaaaaaAaar

@nawaaaaaAaar nawaaaaaAaar commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #570.

Reject MultiIndex level names containing whitespace before serializing the CF compress attribute. Its level names are space-separated, so encoding a level such as "lat lon" currently succeeds but decoding then looks for nonexistent "lat" and "lon" coordinates.

The error names the offending level and tells the caller to rename it. Ordinary Unicode names remain supported; no arbitrary restriction to ASCII or Python identifiers is introduced.

Reproduction

Reproduced on unchanged upstream ee13cb860717299ca8f66d35461e5e50910f3d47: encode a MultiIndex with a "lat lon" level and decode it. Encoding succeeds and decoding raises KeyError: 'lat'. The initial regression matrix failed in all 15 cases before the production change.

Verification

  • python -m pytest cf_xarray/tests/test_coding.py -q: 35 passed on current head.
  • New coverage: spaces, tabs, newlines, leading/trailing whitespace, Unicode whitespace, all three index-selection forms, unchanged input after rejection, and a valid Unicode-name round trip.
  • ruff check cf_xarray/coding.py cf_xarray/tests/test_coding.py: passed.
  • ruff format --check cf_xarray/coding.py cf_xarray/tests/test_coding.py: passed.
  • git diff --check: passed.

This deliberately validates an unrepresentable input instead of silently renaming user coordinates or changing CF's attribute grammar.

Coverage follow-up

The current head also tests that unrelated spaced variable names remain allowed and that unselected MultiIndexes are not validated, and documents the restriction in the coding guide.

A local coverage run confirms all newly added executable statements are covered. The only uncovered lines in coding.py are pre-existing lines 50, 113, 116, 133, 134, 138, and 139. This run passed all 35 coding tests.

Codecov currently reports 75% aggregate patch coverage while its unittests flag reports 100% patch coverage, and warns that the report is 132 commits behind main. The aggregate patch gate remains unresolved; no threshold or configuration has been weakened to bypass it.

Copilot AI balanced review requested due to automatic review settings October 22, 2026 14:31

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

@codecov

codecov Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.12%. Comparing base (a9cebee) to head (43d3a2c).
⚠️ Report is 132 commits behind head on main.

Files with missing lines Patch % Lines
cf_xarray/coding.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #676      +/-   ##
==========================================
+ Coverage   85.78%   87.12%   +1.33%     
==========================================
  Files          13       16       +3     
  Lines        2364     3355     +991     
  Branches      183      313     +130     
==========================================
+ Hits         2028     2923     +895     
- Misses        303      386      +83     
- Partials       33       46      +13     
Flag Coverage Δ
mypy 46.02% <0.00%> (+7.49%) ⬆️
unittests 92.84% <100.00%> (-1.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nawaaaaaAaar

Copy link
Copy Markdown
Contributor Author

Follow-up coverage check: the current Codecov comment reports 75% combined patch coverage, but its unittests flag reports 100% patch coverage and mypy reports 0%; CI/typing uploads are still pending. A fresh local coding-suite coverage run passes all 35 tests and covers all new executable validation lines (55-57). I have not weakened coverage configuration or changed production behavior to work around the aggregate report. The two added scope controls and coding-guide update are in 43d3a2c.

@dcherian
dcherian added this pull request to the merge queue Oct 22, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Oct 23, 2026

This branch has not been deployed

No deployments
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.

assert no variable with spaces in name is used for compression by gathering

2 participants