Skip to content

bpo-40059: tomllib #2

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.

Sign up for GitHub

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

Jump to bottom
Closed
hukkin wants to merge 10 commits into from
Closed

bpo-40059: tomllib #2

hukkin wants to merge 10 commits into from

Conversation

@hukkin
Copy link
Owner

@hukkin hukkin commented Jan 13, 2022 •
edited
Loading

Steps taken

  • Move everything in tomli:src/tomli to Lib/tomllib. Exclude py.typed.

  • Remove __version__ = ... line from Lib/tomllib/__init__.py

  • Move everything in tomli:tests to Lib/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:

    import unittest

from . import load_tests


unittest.main()

  • Add the following to Lib/test/test_tomllib/__init__.py:

    import os
from test.support import load_package_tests

def load_tests(*args):
    return load_package_tests(os.path.dirname(__file__), *args)

    Also change import tomli as tomllib to import tomllib.

  • In cpython/Lib/tomllib/_parser.py replace __fp with fp and __s with s. Add the / to load and loads function signatures.

  • Run make regen-stdlib-module-names

  • Create Doc/library/tomllib.rst and reference it in Doc/library/fileformats.rst

@hukkin
Copy link
Owner Author

hukkin commented Jan 13, 2022 •
edited
Loading

Hey @encukou and @hauntsaninja !

The tomllib PEP is obviously not accepted yet, but I had some spare time so integrated Tomli to the stdlib. Here's the results. I made minimal docs already too.

I've included all Tomli's tests, which means almost 1MB of test data. @encukou perhaps you can tell if this is too much for a single cpython module?

EDIT: The Files changed tab of this PR on GitHub kills my browser (probably due to the amount of test files). Here's a link to it but with test data filtered: https://github.com/hukkin/cpython/pull/2/files?file-filters%5B%5D=.h&file-filters%5B%5D=.md&file-filters%5B%5D=.py&file-filters%5B%5D=.rst&file-filters%5B%5D=No+extension&show-viewed-files=true

hauntsaninja
hauntsaninja reviewed Jan 13, 2022
View reviewed changes
Copy link

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

This is awesome!
encukou will be able to give you better advice about tests, but I believe for zoneinfo and importlib.metadata some tests with extra deps or fixtures exist outside the CPython code base.

@encukou
Copy link

encukou commented Jan 26, 2022

Ah, here's the notification I missed. Sorry for that!
1MB is big, but not giant. I think it's OK to include it.

@hukkin hukkin mentioned this pull request Jan 27, 2022
Closed
merwok
merwok reviewed Jan 27, 2022
View reviewed changes
@hukkin
Copy link
Owner Author

hukkin commented Jan 27, 2022

Thanks a lot for the review @merwok ! I'll have a closer look a bit later.

I think some of the comments have to do with the fact that I tried to minimize the steps needed to take to go from Tomli (backport) to tomllib (stdlib) in order to minimize maintenance effort. Perhaps that's not something I should do though?

@encukou
Copy link

encukou commented Jan 27, 2022

I wouldn't worry about the extra comments, if they simplify maintaining tomli (which will now serve as a backport to earlier versions). It might be good to add an additional comment mentioning that there's a separately maintained copy.

__version__ in stdlib does cause minor problems sometimes, it's better to remove it.

@merwok
Copy link

merwok commented Jan 27, 2022

I was not thinking of the backport! Then I agree with Petr, except for the license text.

merwok
merwok reviewed Jan 27, 2022
View reviewed changes
Doc/library/tomllib.rst Outdated
Copy link

@merwok merwok Jan 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

load wants a binary stream but loads a text string?

If this is intentional, it may be worth pointing out here.

Copy link
Owner Author

@hukkin hukkin Jan 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That's correct: https://www.python.org/dev/peps/pep-0680/#type-accepted-as-the-first-argument-of-tomllib-loads

encukou
encukou reviewed Feb 2, 2022
View reviewed changes
@hukkin hukkin changed the title Tomllib bpo-40059: tomllib Feb 9, 2022
JelleZijlstra
JelleZijlstra reviewed Feb 9, 2022
View reviewed changes
@hukkin hukkin mentioned this pull request Feb 22, 2022
Merged
@hukkin
Copy link
Owner Author

hukkin commented Feb 22, 2022

Closing in favour of python#31498

@hukkin hukkin closed this Feb 22, 2022
@hukkin hukkin mentioned this pull request Dec 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers

@encukou encukou encukou left review comments

@merwok merwok merwok left review comments

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments

@hauntsaninja hauntsaninja hauntsaninja left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@hukkin @encukou @merwok @JelleZijlstra @hauntsaninja