Conversation
Signed-off-by: Cristian Le <git@lecris.dev>
There was a problem hiding this comment.
Code Review
This PR enables ruamel.yaml.clib by default by adding it as a dependency and setting the default YAML type to 'safe'. A suggestion is provided for tmt/utils/__init__.py to improve conciseness. Add a release note for this performance improvement.
| yaml_type = yaml_type or "safe" | ||
| yaml = YAML(typ=yaml_type) |
| "pygments>=2.7.4", | ||
| "requests>=2.25.1", | ||
| "ruamel.yaml>=0.16.6", | ||
| "ruamel-yaml-clib", |
There was a problem hiding this comment.
Making ruamel-yaml-clib an unconditional dependency will break installations on non-CPython implementations (PyPy, Jython, etc.) since it's a C extension only available for CPython. The removed marker in pylock.toml (line 2405) previously restricted it to platform_python_implementation == 'CPython'.
This should be:
"ruamel-yaml-clib; platform_python_implementation == 'CPython'",Without this marker, pip install will fail on PyPy and other implementations where the C extension cannot be compiled/installed.
| "ruamel-yaml-clib", | |
| "ruamel-yaml-clib; platform_python_implementation == 'CPython'", |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Interesting observation. We are implying CPython in many other places though due to other dependencies. Will keep it open for discussions
|
/packit test |
It seems the only thing we need to do to enable this is to pull in this dependency and switch to using
safetype by default. This would be a better apple-to-apple comparison with #4486 since that one uses a C library for handling json.Reference:
Pull Request Checklist
Related to #4406