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

Allow tomli version 2 to be used #46

Closed
wants to merge 1 commit into from

Conversation

Numerlor
Copy link

The current dependency specification disallows versions of tomli >=2, looking at the code that was changed #43, it don't look like any issues could be caused by allowing its use

@Numerlor Numerlor force-pushed the tomli-allow-v2 branch 2 times, most recently from 16c336b to 0170a7c Compare February 15, 2022 15:13
@eugenetriguba
Copy link
Collaborator

eugenetriguba commented Feb 15, 2022

tomli v2 dropped support for Python 3.6, which the project currently supports. That was the main motivation for sticking with v1. We could potentially loosen up the constraint to still allow v2. Dropping 3.6 would be a breaking change though and I assume would require a v2 of taskipy.

@Numerlor
Copy link
Author

Numerlor commented Feb 15, 2022

Is that a problem? From what I can see dependency managers should be able to figure out.
Trying it locally with poetry, using 3.6 as the project version uses tomli 1.2.3 while versions above that use the latest

Seems like the psutil 3.6 file hashes got removed for some reason in the no-update relock though, not sure what that could impact but a local install on windows went without issues

@eugenetriguba
Copy link
Collaborator

eugenetriguba commented Feb 15, 2022

On mobile, I hadn’t actually looked at your PR change when I made that comment, but I see it now. That change you made is what I had meant when I made the comment that we could potentially loosen up the version constraint. Thanks for submitting the PR 🙂

So yes, I assume this should work. We would just need to make sure we’re mindful that users could be running either version and make sure the tests on CI run against both versions.

On @illBeRoy whether he wants to loosen up that constraint. I think it’s okay. There are minor behavior changes with v2 (raising a new error in a certain situation), looking through the change log again, but nothing I see that should cause any issues. It’s nice to make sure end users can use the newer version with the newest version of taskipy.

Does the test suite all pass on v2? CI looks like it's running them on v1 since it is installing from the lockfile.

@illBeRoy
Copy link
Collaborator

Hey @Numerlor, thank you for taking the time in creating this PR :)

Does the test suite all pass on v2? CI looks like it's running them on v1 since it is installing from the lockfile.

That's a great question. We might need to add a full e2e test on each python version in order to truly be able to test how taskipy is being used (that is: create a vanilla project with tasks, use pip to install taskipy and ensure that all works correctly).

Since there aren't any security alerts regarding tomli@1, the way I see it we have two options:

  1. Add that sanity e2e test (feel free to contribute it in this PR, otherwise I can help with that but I'll only have time for that in the weekend). That will help us make sure that there won't be any issues with python 3.6.
  2. Wait with this until we drop support for python 3.6 (not sure when that'll happen); this means that we won't upgrade tomli until we release taskipy@2

wdyt?

@Numerlor
Copy link
Author

Numerlor commented Feb 16, 2022

Seems like the psutil 3.6 file hashes got removed for some reason in the no-update relock though, not sure what that could impact but a local install on windows went without issues

Looks like it was a problem as it couldn't then find the files and had to rebuild, may have been using some caching when I tried it the first time. Fixed it by specifying the different versions for different pythons

@Numerlor
Copy link
Author

Numerlor commented Feb 16, 2022

Trying it on my fork, it does look like the tests use the right version after that update too, I can look into the full testing if it's needed.
Though trying it now locally from a different project, poetry locks the tomli dep to {version = "^1.2.3", markers = "python_version >= \"3.6\" and python_version < \"3.7\""} for some reason, which is the same as it's now so I'm a bit confused as that shouldn't really be possible from a 3.8 environment which I'm in

@eugenetriguba
Copy link
Collaborator

If this PR has stalled because of adding the end to end test, I'd be happy to take a look into it to get this PR through if no one else is already doing so.

@Numerlor
Copy link
Author

Numerlor commented Feb 23, 2022

If this PR has stalled because of adding the end to end test, I'd be happy to take a look into it to get this PR through if no one else is already doing so.

You're welcome to if you want to, I've been meaning to look into it but had my focus elsewhere for a while, and I've been unable to figure out why it's incorrectly locking it when installed.

The generated setup.py seems to be fine at least, so it could just be some issue with poetry when installing from a local project.

...
install_requires = \
['colorama>=0.4.4,<0.5.0', 'psutil>=5.7.2,<6.0.0']

extras_require = \
{':python_version >= "3.6" and python_version < "3.7"': ['tomli>=1.2.3,<2.0.0'],
 ':python_version >= "3.7" and python_version < "4.0"': ['tomli>=2.0.1,<3.0.0'],
 ':sys_platform == "win32"': ['mslex>=0.3.0,<0.4.0']}
...

@illBeRoy
Copy link
Collaborator

Yes, it requires at least one e2e test just to see that we have sanity with all python versions when installing taskipy as a dependency via pip and poetry.

I've been meaning to do so myself but it might take me some time to get to it, so if you want to take it then by all means go ahead :)

@illBeRoy
Copy link
Collaborator

illBeRoy commented Mar 2, 2022

Thanks again for taking your time to improve taskipy @Numerlor !

Following our discussion here, @eugenetriguba created another PR that covers that the correct tomli version is installed: #48

Great initiative :) Looking forward to work with you again!

@illBeRoy illBeRoy closed this Mar 2, 2022
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.

None yet

3 participants