Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
bpo-40059: tomllib#31498
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
bpo-40059: tomllib #31498
Uh oh!
There was an error while loading. Please reload this page.
Conversation
hukkin commented Feb 22, 2022 • edited by encukou
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by encukou
Uh oh!
There was an error while loading. Please reload this page.
hukkin commented Feb 22, 2022
A question: upstream (Tomli) is formatted with Black, using Black's defaults. This means a line length of 88. Should I reformat with line length at 79? |
hugovk commented Feb 22, 2022
This will need NEWS and "What's new" entries: You can add this to https://github.com/python/cpython/blob/main/.github/CODEOWNERS |
mgorny commented Feb 22, 2022
Perhaps it'd make sense to include the tomli version used (or even the git commit hash), to make future syncing easier. |
hukkin commented Feb 22, 2022
It was previously suggested that I remove Tomli version from the standard library port: hukkin#2 (comment) I plan to include a migration guide in Tomli repository, also including commit hash that tracks what is currently in the stdlib. |
TeamSpen210 commented Feb 22, 2022
Would it be good to mention in the docs why |
encukou commented Feb 23, 2022
I should get to the review this week or the next.
No, I don't think that's worth it. And anyway, line length is not the only point where Black disagrees with PEP8 (starting with, like, the whole philosophy).
Let's leave docs improvements to future PRs, so they get a proper discussion and don't delay this PR? |
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Petr Viktorin <[email protected]>
encukou left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few test nitpicks, but I'm happy to merge this!
| if isinstance(expected, MissingFile): | ||
| # Would be nice to xfail here, but unittest doesn't seem | ||
| # to allow that in a nice way. | ||
| continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MissingFile looks unnecessary. Does tomli need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see it does.
For a poor man's xfail, you could you assert that p.stem is one of the expected failing cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MissingFile looks unnecessary. Does tomli need it?
Yeah one of the two external test suites has a couple test cases where the expected data is missing.
For a poor man's xfail, you could you assert that p.stem is one of the expected failing cases.
I'm not sure I understand what you mean. Maybe you can show with the "Add a suggestion" feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something like:
if isinstance(expected, MissingFile): assert valid.stem in ("xfail_test_case1, "xfail_test_case2", ...) continue There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, of course, thanks. Committed that.
| INVALID_FILES = tuple((DATA_DIR / "invalid").glob("**/*.toml")) | ||
| class TestData(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For peace of mind, could you assert len(VALID_FILES) > 0, and same for INVALID_FILES?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Makes sense. Added the asserts.
bedevere-bot commented Mar 3, 2022
hukkin commented Mar 4, 2022
It seems the failing CI job ( |
encukou commented Mar 4, 2022
The alpha 6 release is a bit bumpy and I don't want to destabilize it, so I'm holding off the merge until it's out. |
encukou commented Mar 8, 2022
Let's get it in! |
This adds a new standard library module,
tomllib, for parsing TOML. The recently accepted PEP 680 -- tomllib is relevant here.This PR has already seen some review in a PR under my personal fork: hukkin#2 (thanks to @encukou, @merwok, @hauntsaninja, @JelleZijlstra (I hope I'm not forgetting anyone)).
The implementation is based on Tomli which I plan to keep maintaining as a backport for Python versions 3.7, 3.8, 3.9 and 3.10, until finally Python 3.10 goes EOL.
Steps taken (converting
tomlitotomllib)Move everything in
tomli:src/tomlitoLib/tomllib. Excludepy.typed.Remove
__version__ = ...line fromLib/tomllib/__init__.pyMove everything in
tomli:teststoLib/test/test_tomllib. Exclude the following test data dirs recursively:tomli:tests/data/invalid/_external/tomli:tests/data/valid/_external/Create
Lib/test/test_tomllib/__main__.py:Add the following to
Lib/test/test_tomllib/__init__.py:Also change
import tomli as tomllibtoimport tomllib.In
cpython/Lib/tomllib/_parser.pyreplace__fpwithfpand__swiths. Add the/toloadandloadsfunction signatures.Run
make regen-stdlib-module-namesCreate
Doc/library/tomllib.rstand reference it inDoc/library/fileformats.rstedit: For reference, there's one more step – Add
tomllibto Makefile.pre.inhttps://bugs.python.org/issue40059