-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
bpo-40059: tomllib #31498
Conversation
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? |
This will need NEWS and "What's new" entries: You can add this to https://github.com/python/cpython/blob/main/.github/CODEOWNERS |
Perhaps it'd make sense to include the tomli version used (or even the git commit hash), to make future syncing easier. |
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. |
Would it be good to mention in the docs why |
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? |
Co-authored-by: Petr Viktorin <encukou@gmail.com>
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!
Lib/test/test_tomllib/test_data.py
Outdated
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.
It seems the failing CI job ( |
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. |
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
tomli
totomllib
)Move everything in
tomli:src/tomli
toLib/tomllib
. Excludepy.typed
.Remove
__version__ = ...
line fromLib/tomllib/__init__.py
Move everything in
tomli:tests
toLib/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 tomllib
toimport tomllib
.In
cpython/Lib/tomllib/_parser.py
replace__fp
withfp
and__s
withs
. Add the/
toload
andloads
function signatures.Run
make regen-stdlib-module-names
Create
Doc/library/tomllib.rst
and reference it inDoc/library/fileformats.rst
edit: For reference, there's one more step – Add
tomllib
to Makefile.pre.inhttps://bugs.python.org/issue40059