Skip to content

Fix/ Boolean abbreviations get evaluated in keyword context #415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 9, 2025

Conversation

FoamScience
Copy link
Contributor

@FoamScience FoamScience commented Jul 9, 2025

This is a tiny PR to fix a minor bug with situations where n, f, y, or t appear as a keyword in a key-val tuple.

Original behavior:

The unit tests:

    assert dumps([("a", "b"), ("c", "d"), ("n", "no"), ("y", "yes")]) == b"(a b; c d; n no; y yes;)"
    assert (
        FoamFile.dumps({"a": "b", "c": "d", "n": "no", "y": "yes"})
        == b"{FoamFile {version 2.0; format ascii; class dictionary;}} a b; c d; n no; y yes;"
    )

fail, saying, respectively:

- b'(a b; c d; n no; y yes;)'
+ b'(a b; c d; no no; yes yes;)'
-----------------------------------------
-  b'{FoamFile {version 2.0; format ascii; class dictionary;}} a b; c d; n no; y yes;'
+  b'{FoamFile {version 2.0; format ascii; class dictionary;}} a b; c d; no no; yes yes;'

In short, "n" and "y" get normalized even if they are keywords. This gets annoying for example when controlling nProcs settings for decomposition methods is desired...

Suggested solution:

  • Don't consider these single-char abbreviations as booleans. Effects: All unit tests should still pass; these abbreviations are not supported by OpenFOAM so this should be alright.

An alternative solution that I haven't had time to try is to dig around normalize_data/normalize_keyword to skip normalizing the first element in a tuple but that would be hard to distinguish because of OpenFOAM's ambiguous syntax: without full context, the first "n" in ("n" "n") cannot be judged to be a keyword or a data element.

@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.69%. Comparing base (da0d252) to head (c50798c).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #415      +/-   ##
==========================================
- Coverage   89.74%   89.69%   -0.05%     
==========================================
  Files          22       22              
  Lines        2057     2057              
==========================================
- Hits         1846     1845       -1     
- Misses        211      212       +1     

☔ View full report in Codecov by Sentry.
📢 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.

@gerlero
Copy link
Owner

gerlero commented Jul 9, 2025

@FoamScience thanks!

  • these abbreviations are not supported by OpenFOAM so this should be alright.

Makes total sense to get rid of them then.

@gerlero gerlero merged commit 13a9b16 into gerlero:main Jul 9, 2025
40 checks passed
@gerlero
Copy link
Owner

gerlero commented Jul 9, 2025

@FoamScience released in version 1.1.1. Thanks again!

@FoamScience
Copy link
Contributor Author

Thanks for the quick response! and the quick release! saves me the trouble of requiring a git commit :)

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.

3 participants