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

Migrate dependency toml to tomllib #50

Merged
merged 1 commit into from
Oct 21, 2023

Conversation

Contextualist
Copy link
Contributor

Reasons for the migration:

  1. hukkin/tomli has been accepted as part of the Python standard library (see PEP680), named as tomllib, and it is available since Python 3.11.
  2. uiri/toml has not been active since 2020, while hukkin/tomli is an actively maintained implementation of TOML 1.0 spec, with 100% test coverage.

@coltonbh
Copy link
Collaborator

coltonbh commented Oct 17, 2023

Keen eye on the hukkin/tomli library--it appears this will indeed be better supported since it is the official library added to the standard library. Thanks :)

Let's keep the python 3.11 checks and if/else import statements out. That adds complexity and would require a multi-python-version testing environment which we'll want to avoid. Juice ain't worth the squeeze. This is a very simple feature--just read/write a toml file, we don't want to add complexity to the package to save a tiny package install for 3.11 users.

For merge see my review comments in the code, specifically:

  1. Remove python3.11 checks and just install tomli and tomli_w for all python versions.
  2. Don't alias the import
  3. Fix file read/write using "b" (binary) mode, these are text files, not binary.

Thanks for the maintenance! Any questions lemme know ❤️

P.S. - Also squash your commits so there is only 1 commit after you make these changes. Google a how-to. If you need help let me know :)

@Contextualist
Copy link
Contributor Author

Thanks for the quick review!

Let's keep the python 3.11 checks and if/else import statements out.

I am following the suggestions from the authors (here and here) for the compatibility layer. I would say that it's OK to trust the maintainer for the consistency between tomllib and hukkin/tomli, but it would also be OK to always install tomli until we no longer need to support Python < 3.11, though.

Fix file read/write using "b" (binary) mode, these are text files, not binary.

Binary mode is required by the tomllib interface, in order to enforce UTF-8 encoding as part of the TOML spec compliance.

Let me know your decisions and I will make one final squash commit.

@coltonbh
Copy link
Collaborator

coltonbh commented Oct 18, 2023

Thanks for the links to the documentation!

Ok I'm persuaded by the compatibility layer for 3.11 and the rb mode. The rb is very surprising. I've never seen that for text file formats before.

So to finish this up we need a to add a 3.11 environment to the CI GitHub pipeline to ensure correctness and functionality for python 3.11+.

Let's finish up with this:

  1. Change the tests workflow in test.yaml to use a matrix of python versions and include 3.8 and 3.11 in the matrix so our tests run on both versions. (google matrix tests for GitHub actions to see how this is done).
  2. Add a note inside the CHANGELOG.md under ## Unreleased ### Changed that notes the changes you made.
  3. Squash commits.

I'll take it from there and merge and release a new version.

Thank you!

@coltonbh
Copy link
Collaborator

coltonbh commented Oct 18, 2023

Also, please rebase from main as I've updated GitHub actions to run on pull requests from forked repos. Thanks!

@Contextualist Contextualist force-pushed the tomllib-migrate branch 2 times, most recently from 6ab106e to d89c31e Compare October 18, 2023 23:21
@Contextualist
Copy link
Contributor Author

Done! Let me know if there is any additional concern.

@coltonbh coltonbh merged commit 6fbf1c4 into mtzgroup:main Oct 21, 2023
8 checks passed
@coltonbh
Copy link
Collaborator

Very professional! Looks great! Congrats on your first PR contribution ☺️

@coltonbh
Copy link
Collaborator

Released to PyPi as v0.8.3. Thanks, @Contextualist .

@Contextualist Contextualist deleted the tomllib-migrate branch October 21, 2023 01:20
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.

2 participants