Preserve case when parsing boolean like strings#1634
Conversation
This commit updates the YAML parser so that input strings (for example, TRUE) retain their case, and are not converted into Python equivalents (i.e. True).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces syaml_bool to preserve the original string representation of booleans (such as 'TRUE' or 'False') when parsing and dumping YAML files, and updates the validator, expander, and variant modules to support it. The review feedback highlights a duplicate and buggy definition of represent_bool in OrderedLineDumper inside spack_yaml.py, and recommends replacing fragile string-based type checks (type(...).__name__ == "syaml_bool") in variants.py with robust isinstance checks.
| def represent_bool(self, data): | ||
| if hasattr(data, 'string_val') and data.string_val: | ||
| return self.represent_scalar('tag:yaml.org,2002:bool', data.string_val) | ||
| return super(SafeDumper, self).represent_bool(bool(data)) | ||
|
|
There was a problem hiding this comment.
There is a duplicate definition of represent_bool in OrderedLineDumper. Additionally, this first definition incorrectly uses super(SafeDumper, self) instead of super(OrderedLineDumper, self), which would raise a TypeError at runtime if it were ever called. Since it is shadowed by the correct definition further down, this duplicate should be removed entirely to avoid confusion and potential bugs.
| if default_var and ( | ||
| isinstance(default_var.default, bool) | ||
| or type(default_var.default).__name__ == "syaml_bool" | ||
| ): |
There was a problem hiding this comment.
Using type(default_var.default).__name__ == "syaml_bool" is a fragile string-based type check. It is more robust and idiomatic to import syaml_bool and use isinstance to check the type, which also correctly handles subclasses.
from spack.util.spack_yaml import syaml_bool
if default_var and isinstance(default_var.default, (bool, syaml_bool)):|
|
||
| def _build_definitions(self) -> tuple: | ||
| if isinstance(self.default, bool): | ||
| if isinstance(self.default, bool) or type(self.default).__name__ == "syaml_bool": |
There was a problem hiding this comment.
Using type(self.default).__name__ == "syaml_bool" is a fragile string-based type check. It is more robust and idiomatic to import syaml_bool and use isinstance to check the type.
| if isinstance(self.default, bool) or type(self.default).__name__ == "syaml_bool": | |
| from spack.util.spack_yaml import syaml_bool | |
| if isinstance(self.default, (bool, syaml_bool)): |
| def format_value(self, value: Any) -> str: | ||
| """Format a value for this variant into Spack-like syntax""" | ||
| if isinstance(self.default, bool): | ||
| if isinstance(self.default, bool) or type(self.default).__name__ == "syaml_bool": |
There was a problem hiding this comment.
Using type(self.default).__name__ == "syaml_bool" is a fragile string-based type check. It is more robust and idiomatic to import syaml_bool and use isinstance to check the type.
| if isinstance(self.default, bool) or type(self.default).__name__ == "syaml_bool": | |
| from spack.util.spack_yaml import syaml_bool | |
| if isinstance(self.default, (bool, syaml_bool)): |
Ramble Performance Test MetricsResults produced with commit: 58252c2
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1634 +/- ##
========================================
Coverage 93.19% 93.19%
========================================
Files 345 345
Lines 33456 33474 +18
========================================
+ Hits 31178 31196 +18
Misses 2278 2278 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
This merge adds a test, and fixes an issue where input strings (for example in env-vars) that can be converted into a boolean (i.e. TRUE) are converted into Python syntax for the booleans (i.e. True).
They now retain their original input case.