Skip to content

Swap in tomllib/tomli for toml. #340

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
Merged
merged 4 commits into from
Nov 24, 2023
Merged

Swap in tomllib/tomli for toml. #340

merged 4 commits into from
Nov 24, 2023

Conversation

sacsar
Copy link
Contributor

@sacsar sacsar commented Nov 23, 2023 •
edited
Loading

Description

Fixes #339 -- the toml package does not support heterogenous arrays, which are now allowed in the toml spec. This commit swaps in tomllib from the standard library for Python >= 3.11 and its backport tomli for older versions.

Checklist:

  • I have updated the documentation in the README.md file or my changes don't require an update.
  • I have added an entry in CHANGELOG.md.
  • I have added or adapted tests to cover my changes.
  • I have run tox -e fix-style to format my code and checked the result with tox -e style.

@sacsar
Swap tomllib for toml.
6c0c667 
Fixes jendrikseipp#339 -- the toml package does not support heterogenous arrays,
which are now allowed in the toml spec. This commit swaps in tomllib
from the standard library for Python >= 3.11 and its backport tomli for
older versions.
@sacsar
Update changelog with PR reference.
4b2a421
@sacsar
Copy link
Contributor Author

sacsar commented Nov 23, 2023

Note: While the tox style check passes, flake8 finds errors when run via pre-commit (including in vulture/core.py, which I didn't touch). I believe this is flake8 and black fighting, but I didn't go too far down the rabbit hole and ended up using --no-verify.

jendrikseipp
jendrikseipp requested changes Nov 24, 2023
View reviewed changes
Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Code looks good, I only have a few minor nitpicks.

sacsar and others added 2 commits November 24, 2023 10:10
@sacsar
Copy link
Contributor Author

sacsar commented Nov 24, 2023

Note: While the tox style check passes, flake8 finds errors when run via pre-commit (including in vulture/core.py, which I didn't touch). I believe this is flake8 and black fighting, but I didn't go too far down the rabbit hole and ended up using --no-verify.

It turns out this isn't a conflict with black, it's a change in flake8 between Python versions. When I run tox from a 3.12 virtualenv, the style check fails with:

tests/test_utils.py:146:32: E272 multiple spaces before keyword
vulture/config.py:119:69: E231 missing whitespace after ','
vulture/config.py:120:25: E231 missing whitespace after ','
vulture/config.py:120:30: E231 missing whitespace after ','
vulture/config.py:120:42: E231 missing whitespace after ','
vulture/config.py:130:59: E231 missing whitespace after ','
vulture/config.py:138:71: E231 missing whitespace after ','
vulture/core.py:158:41: E231 missing whitespace after ':'
vulture/core.py:172:50: E231 missing whitespace after ':'

When run from a 3.8 virtualenv, flake8 is happy. I was getting failures only from pre-commit because I had installed pre-commit from the 3.12 virtualenv and then switched virtualenvs.

It turns out that if you use pyenv and have pre-commit (or other utilities) installed with a Python it manages, it turns into a pain in the butt, so I ended up installing pre-commit via pipx with my system python (3.11.5). It turns out there is a bug in a recent version of flake8 that was fixed in 6.1.0.

tl;dr @jendrikseipp I think the fix is just to bump flake8 in tox.ini. Do you want a separate PR in the name of a clean changelog or should I just throw it in here?

@jendrikseipp
Copy link
Owner

Thanks for the changes and for looking into the flake8 issue! I'll merge this as is to have a clean history. Would be great if you could send another PR for the flake8 issue.

@jendrikseipp jendrikseipp merged commit 072c3bb into jendrikseipp:main Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jendrikseipp jendrikseipp jendrikseipp requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Switch to tomli/tomllib for toml parsing
2 participants
@sacsar @jendrikseipp