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

Add support for tomli to parse pyproject.toml #2014

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mattusifer
Copy link

PR Summary

PR checklist

Please make sure that the following things have been addressed (and check the relevant checkboxes):

  • Commits respect our guidelines
  • Tests are passing properly (see here on how to run Elpy's tests)

For new features only:

  • Tests has been added to cover the change
  • The documentation has been updated

@gopar
Copy link
Collaborator

gopar commented Jul 21, 2023

why remove the built in toml parser?
Also you are now forcing a dependency on tomli if we add this in

@mattusifer
Copy link
Author

This doesn't remove support for toml, it adds support for tomli. It's not forced, it is imported in the same "optional" way as toml:

try:
    import tomli
except ImportError:
    tomli = None

Currently the toml lib is the only way elpy can parse pyproject.toml files, which makes it required, but toml isn't always in the environment. I think it would make sense to provide other options. Pip itself uses tomli to parse pyproject.toml files, and tomllib (a derivative of tomli) was recently added to the python standard library in 3.11 (so ultimately it will be best to just use that).

This PR isn't ready (yet). I didn't have toml in one of my venvs so I ran into this issue, still figuring out the best way to resolve it generally.

@gopar
Copy link
Collaborator

gopar commented Jul 21, 2023

Ah gotcha, completely misread the code. Ping whenever you want a review :)

@steinitzu
Copy link
Contributor

Is anything blocking this? Would love to have this as I currently have to patch my blackutil manually any time I update elpy.

Btw tomli is a dependency of black so imo fine to make it the only option. If you're using black, you have it installed.

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

Successfully merging this pull request may close these issues.

3 participants