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

bpo-40059: tomllib #2

Closed
wants to merge 10 commits into from
Closed

bpo-40059: tomllib #2

wants to merge 10 commits into from

Conversation

hukkin
Copy link
Owner

@hukkin hukkin commented Jan 13, 2022

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

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

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.

Doc/library/tomllib.rst Outdated Show resolved Hide resolved
Doc/library/tomllib.rst Outdated Show resolved Hide resolved
Lib/test/test_tomllib/__init__.py Show resolved Hide resolved
Lib/test/test_tomllib/burntsushi.py Show resolved Hide resolved
Lib/tomllib/__init__.py Show resolved Hide resolved
Lib/tomllib/__init__.py Outdated Show resolved Hide resolved
Lib/tomllib/__init__.py Outdated Show resolved Hide resolved
Doc/library/tomllib.rst Outdated Show resolved Hide resolved
@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.


import tomllib

toml_str = """
Copy link

@merwok merwok Jan 27, 2022

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

Choose a reason for hiding this comment

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

Doc/library/tomllib.rst Outdated Show resolved Hide resolved
@hukkin hukkin changed the title Tomllib bpo-40059: tomllib Feb 9, 2022
@hukkin
Copy link
Owner Author

hukkin commented Feb 22, 2022

Closing in favour of python#31498

@hukkin hukkin closed this Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants