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

Convert to using tomli #3830

Closed
henryiii opened this issue Mar 2, 2022 · 8 comments · Fixed by #3831
Closed

Convert to using tomli #3830

henryiii opened this issue Mar 2, 2022 · 8 comments · Fixed by #3831
Assignees
Labels
enhancement Making something better
Milestone

Comments

@henryiii
Copy link

henryiii commented Mar 2, 2022

The library currently depends on the dead "toml" library, instead of the "tomli" library (which was just accepted into the stdlib as "tomllib" for Python 3.11. It should be a really easy swap; it's almost the same API as toml, it just expects binary files instead of unicode ones if using toml{i,lib}.load. loads is the same. Writing is a separate library.

Working on pyodide at pyodide/pyodide#2234 and don't want to have to port a dead library.

@welcome
Copy link

welcome bot commented Mar 2, 2022

Hi, and welcome to yt! Thanks for opening your first issue. We have an issue template that helps us to gather relevant information to help diagnosing and fixing the issue.

@matthewturk
Copy link
Member

@henryiii Thanks for the issue -- and I agree, this is the right thing to do. Do you happen to know if there is an idiomatic way to handle it for existing installations that may already have toml but not tomli?

@henryiii
Copy link
Author

henryiii commented Mar 2, 2022

Sadly, yes: https://github.com/pypa/build/blob/5d711b4cb24b2d9ed0884a5a9ba54252440899a2/src/build/__init__.py#L44-L49

Though otherwise the PyPA completely moved over to tomli for pip, setuptools, etc (except for build, we needed to keep it as bootstrappable as possible so that abomination is still in there).

Do you happen know what non-optional requirements in install_requires are actually optional?

@henryiii
Copy link
Author

henryiii commented Mar 2, 2022

(That's going to be extra fun once it has a if sys.version_info < (3, 11) toggle around a tomli/tomllib choice - waiting for a tomllib implementation to be available, though)

@matthewturk matthewturk self-assigned this Mar 2, 2022
@neutrinoceros
Copy link
Member

Do you happen know what non-optional requirements in install_requires are actually optional?

@henryiii I was about to say "none", but after a more carful check I think pyyaml is actually optional. I believe it's the only one though.

I was aware of PEP 680 being accepted and the toml package being unsupported, but there was no clear benefit from migrating early. Now that there is, I guess this should be done in time for our next release.

Do you happen to know if there is an idiomatic way to handle it for existing installations that may already have toml but not tomli?

I'm curious what's the gain we expect here ? Why not just move from a hard dependency to another ?

@henryiii
Copy link
Author

henryiii commented Mar 2, 2022

I'd highly recommend the hard dependency.

There's not much here:
https://github.com/pyodide/pyodide/blob/ab08812ca564738966cd57024f0931c065a461e5/packages/yt/meta.yaml#L14-L17

Ahh, it looks like a lot got added for 4.0. And 3.x doesn't support matplotlib 3.5. Okay. :/

@henryiii
Copy link
Author

henryiii commented Mar 2, 2022

Is IPython actually required?

@neutrinoceros
Copy link
Member

Is IPython actually required?

I thought it was, but I don't see it being imported at the top-level in any module actually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants