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

Add conditional tomllib support for Python 3.11+ #11052

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

murtazahussainii
Copy link

What are you trying to accomplish?

Anything you want to highlight for special attention from reviewers?

How will you know you've accomplished your goal?

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@murtazahussainii murtazahussainii requested a review from a team as a code owner December 4, 2024 05:04
Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Thanks, the import changes make sense.

However, for the formatting changes, can you please break them out as a separate PR? Otherwise someone hitting git blame will think those changes are related to tomllib, but they're not. 😃

@jeffwidman
Copy link
Member

Also, looks like there might be a test failure, probably tomllib related, but it'll be easier for you to figure out/fix if this PR is scoped to just the tomllib change, and then do the formatting changes (which we're probably open to) in a separate PR

Copy link
Contributor

@ulgens ulgens left a comment

Choose a reason for hiding this comment

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

Wouldn't this require a change in requirements file too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants