Support legacy ruamel.yaml loaders #1060
Conversation
ischoegl
force-pushed
the
fix-dated-ruamel
branch
from
34ad946
to
21d7121
Compare
interfaces/cython/cantera/ck2yaml.py
Outdated
| try: | ||
| yaml_ = yaml.YAML(typ="rt") | ||
| with open(path, 'rt', encoding="utf-8") as stream: | ||
| yml = yaml_.load(stream) | ||
| except yaml.constructor.ConstructorError: | ||
| with open(path, "rt", encoding="utf-8") as stream: | ||
| # Ensure that the loader remains backward-compatible with legacy | ||
| # ruamel.yaml versions (prior to 0.17.0). | ||
| yaml.round_trip_load(stream) |
There was a problem hiding this comment.
I'd suggest passing a pathlib.Path instance directly to load(), rather than using the context manager. I'm not sure what version started supporting that, but hopefully something that's recent enough...
Also, in the last line of the except clause, you don't assign the return value to a variable.
There was a problem hiding this comment.
whoops - fixed the assignment. Regarding pathlib.Path, I don't think that the function receives the correct argument (from what I see it's a string). For the time being, I hope to just fix #1059 as I inadvertently broke the build for older ruamel.yaml versions - getting rid of os in ck2yaml is a slightly larger project (it currently doesn't use pathlib at all).
| def load_yaml(yml_file): | ||
| # Load YAML data from file using the "safe" loading option. | ||
| try: | ||
| yaml_ = yaml.YAML(typ="safe") | ||
| with open(yml_file, "rt", encoding="utf-8") as stream: | ||
| return yaml_.load(stream) | ||
| except yaml.constructor.ConstructorError: | ||
| with open(yml_file, "rt", encoding="utf-8") as stream: | ||
| # Ensure that the loader remains backward-compatible with legacy | ||
| # ruamel.yaml versions (prior to 0.17.0). | ||
| yaml.safe_load(stream) |
There was a problem hiding this comment.
Same 2 comments here.
One additional comment is, why remove the type argument?
There was a problem hiding this comment.
For simplicity - all of the tests use the safe loader at the moment.
Upstream ruamel.yaml changes loaders in 0.17.0, with earlier loaders being deprecated. However, the replacement loaders were introduced with the same version. This update ensures that both loaders are being supported.
ischoegl
force-pushed
the
fix-dated-ruamel
branch
from
21d7121
to
4dec05e
Compare
|
@bryanwweber … any further comments? As mentioned, I’d prefer to not start switching to |
Changes proposed in this pull request
A recent change in #1049 replaces a deprecated YAML loader. However, the replacement loader was only implemented for the most recent ruamel.yaml versions.
If applicable, fill in the issue number this pull request is fixing
Fixes #1059
If applicable, provide an example illustrating new features this pull request is introducing
Checklist
scons build&scons test) and unit tests address code coverage