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

Replace vendored toml with tomli for TOML v1.0.0 compat #10035

Merged
merged 6 commits into from
Jul 12, 2021
Merged

Replace vendored toml with tomli for TOML v1.0.0 compat #10035

merged 6 commits into from
Jul 12, 2021

Conversation

hukkin
Copy link
Contributor

@hukkin hukkin commented Jun 1, 2021

Closes #10034

Copy link
Contributor Author

@hukkin hukkin left a comment

Choose a reason for hiding this comment

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

I've got a few questions/notes here myself!

src/pip/_vendor/README.rst Show resolved Hide resolved
src/pip/_vendor/tomli/__init__.py Show resolved Hide resolved
@pradyunsg
Copy link
Member

You should run nox -s vendoring, which runs the automation we have for doing the vendoring.

The CI job for vendoring runs it as well, and shows how what you've made in the patch is different from what the automation generates.

@hukkin
Copy link
Contributor Author

hukkin commented Jun 1, 2021

You should run nox -s vendoring

Thanks! For reasons unknown to me, nox failed to install vendoring:

ERROR: No matching distribution found for vendoring>=0.3.0

But tox -e vendoring worked like a charm.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

Let's hold off on merging this until tomli stabilises. I don't think there's a hurry in vendoring this, and given the current frequency of enhancements being made to tomli's error reporting and edge case handling, it can't hurt to wait until closer to our release to update and merge this.

news/10034.feature.rst Outdated Show resolved Hide resolved
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

One nit, otherwise LGTM.

@hukkin
Copy link
Contributor Author

hukkin commented Jun 7, 2021

Let's hold off on merging this until tomli stabilises...

Makes sense. At this stage the parser passes all tests by BurntSushi and iarna plus a good amount of extra tests specially crafted for cases the aforementioned test suites did not cover. So I'm fairly confident and have stopped actively searching for bugs now. That said, the parser is still brand new...

One nit, otherwise LGTM.

Thanks, commited!

@pradyunsg
Copy link
Member

@hukkin Could you squash the commits in this PR, to be something like the following?

  • Vendor tomli v1.0.3
  • Replace toml usage with tomli
  • Unvendor toml, since it's no longer used
  • {all the other misc changes}

This would separate manual changes from automation-driven changes which is basically always a good idea -- that'll make it easier to manage this PR, making it easier to ensure that the individual changes are accurate.

@pradyunsg pradyunsg dismissed their stale review July 1, 2021 20:53

This is no longer a concern.

@hukkin
Copy link
Contributor Author

hukkin commented Jul 1, 2021

Just pushed an improved commit history @pradyunsg

@uranusjr
Copy link
Member

@pypa/pip-committers do we want this in 21.2?

@pradyunsg
Copy link
Member

I like how that sounds! The sooner we adopt TOML 1.0, the sooner the rest of the ecosystem can start adopting it.

@uranusjr
Copy link
Member

Let’s do it then. I’ll merge as soon as @hukkin confirms this is good to go.

@hukkin
Copy link
Contributor Author

hukkin commented Jul 12, 2021

@uranusjr This is ready to merge from my side

@uranusjr
Copy link
Member

Awesome!

@uranusjr uranusjr merged commit bd5ac26 into pypa:main Jul 12, 2021
@hukkin hukkin deleted the tomli branch July 12, 2021 09:00
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vendor a TOML v1.0.0 compatible TOML parser
4 participants