mtzgroup / chemcloud-client

Python client for TeraChem Cloud
MIT License
11 stars 3 forks source link

Migrate dependency `toml` to `tomllib` #50

Closed Contextualist closed 10 months ago

Contextualist commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 10 months ago

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 commented 10 months ago

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

Contextualist commented 10 months ago

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

coltonbh commented 10 months ago

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

coltonbh commented 10 months ago

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