Skip to content
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

yq: use tomllib if available #185

Closed
wants to merge 1 commit into from
Closed

Conversation

intelfx
Copy link

@intelfx intelfx commented Mar 8, 2024

I'm not sure if tomlkit gives any sort of extended representation that is specially handled for the TOML->TOML roundtrip case, so this tries to be smart and only uses tomllib if it is available AND output format is not TOML.

(One dirty commit for now, I'll make separate commits if the entire thing makes sense.)

Fixes #184.

Copy link
Owner

@kislyuk kislyuk left a comment

Choose a reason for hiding this comment

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

This looks good, thanks. Can you transition out of draft and make any other changes you planned? It would be great to add a test that asserts on the new error messages too.

@intelfx
Copy link
Author

intelfx commented Mar 11, 2024

This looks good, thanks. Can you transition out of draft and make any other changes you planned? It would be great to add a test that asserts on the new error messages too.

Okay, I'll do it when I get around to this again.

One question, is there actually any sort of special style-preserving handling for TOML->TOML roundtrip (like there is for YAML with yq -Y), or should I not try to be smart and just use tomllib when it's available and only fall back to tomlkit if running on <3.11? I didn't find any but I wasn't looking very hard.

@kislyuk
Copy link
Owner

kislyuk commented Mar 12, 2024

Thanks for asking. Part of the reason I went with tomlkit is that it preserves style information - but I never got around to plumbing the comments and whitespace using __yq metadata keys through the JSON representation, as we do with yq -Y.

Given the fact that I don't intend to actively invest in further TOML support, I think it's time to switch things up:

  • When installing on Python 3.8-3.10, depend on tomli and tomli-w
  • When installing on Python 3.11+, depend on tomli-w
  • Lazily Import tomllib or tomli, conditionally on sys.python_version, in decode_toml()
  • Lazily import tomli-w in encode_toml()

@kislyuk
Copy link
Owner

kislyuk commented Apr 14, 2024

Thanks for your input here. I went ahead and implemented the changes you suggested, only when using Python 3.11+ and not round-tripping (i.e. when using tomlq without the -t option). I realized I needed to stay with tomlkit when using the -t option due to a variety of usability issues with other options, and I will not be introducing any new dependencies (i.e. tomli) to the package, so this change will remain exclusive to Python 3.11+.

I released the changes in yq-3.3.0. In my testing, this yields a roughly 2x speedup when using tomlq to parse many small TOML files in a loop.

I'm going to close this PR now - let me know if there's anything I missed.

@kislyuk kislyuk closed this Apr 14, 2024
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.

TOML support (tomlkit) is very slow
2 participants