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

Use tomllib on Python 3.11+ #1152

Closed
wants to merge 1 commit into from
Closed

Conversation

felixonmars
Copy link
Contributor

Fallback to tomli for compatibility with older Python

Fallback to tomli for compatibility with older Python
@Spitfire1900
Copy link
Contributor

Would you like to do similar for importlib-metadata as part of this PR? The PyPi package is only required on Python 3.7.

@@ -52,11 +52,14 @@ def _GetExcludePatternsFromPyprojectToml(filename):
"""Get a list of file patterns to ignore from pyproject.toml."""
ignore_patterns = []
try:
import tomli as tomllib
Copy link
Contributor

@Spitfire1900 Spitfire1900 Sep 24, 2023

Choose a reason for hiding this comment

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

Not a fan of nested try/catches in favor of https://peps.python.org/pep-0484/#version-and-platform-checking style approach.

I'm also wondering if moving the import to ./yapf/yapflib/__init__.py is a good idea idea vs. conditional login in every file.

Lastly, this package is now part of the package requirements, so managing an ImportError may no longer be applicable.

import sys
if sys.version_info >= (3, 11):
    import tomllib
else:
    import tomli as tomllib

Copy link
Contributor

@Spitfire1900 Spitfire1900 left a comment

Choose a reason for hiding this comment

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

Use sys.version_info instead of nested try/catches.

Strip all import try/catches as tomli / tomllib is guaranteed by requirements specification.

@felixonmars
Copy link
Contributor Author

Looks like requested changes are in #1158 now. Closing in favor of that one.

@felixonmars felixonmars closed this Oct 1, 2023
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.

2 participants