Skip to content

Conversation

Woklex
Copy link
Contributor

@Woklex Woklex commented Dec 24, 2020

Poetry now needs tomlkit version higher than 0.7 https://github.com/python-poetry/poetry/blob/bf30ca696aa47bcc42937910d620c95c95c0741e/pyproject.toml#L38. If you leave everything as it is, then it will not install commitizen and poetry with a version greater than 1.1.0

Description

Update tomlkit version requirement

Checklist

  • Add test cases to all the changes you introduce
  • Run ./script/format and ./script/test locally to ensure this change passes linter check and test
  • Test the changes on the local machine manually
  • Update the documentation for the changes

Expected behavior

Install the latest version poetry and commitizen

Steps to Test This Pull Request

Additional context

@codecov
Copy link

codecov bot commented Dec 24, 2020

Codecov Report

Merging #321 (b43a7cb) into master (aa2959d) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #321   +/-   ##
=======================================
  Coverage   97.17%   97.17%           
=======================================
  Files          35       35           
  Lines         991      991           
=======================================
  Hits          963      963           
  Misses         28       28           
Flag Coverage Δ
unittests 97.17% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
commitizen/__version__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edc06c3...b43a7cb. Read the comment docs.

termcolor = "^1.1"
packaging = ">=19,<21"
tomlkit = "^0.5.3"
tomlkit = ">=0.5.3,<1.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

If my understanding is not wrong, this is basically the same as what is was
https://python-poetry.org/docs/dependency-specification/#version-constraints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example from the documentation:
^0.2.3 | >=0.2.3 <0.3.0

Installation example:

root@38df9bc1e4e0:/# pip install poetry==1.1.4 commitizen==2.11.1
Collecting commitizen==2.11.1
  Downloading commitizen-2.11.1-py3-none-any.whl (42 kB)
     |████████████████████████████████| 42 kB 228 kB/s
Collecting poetry==1.1.4
  Downloading poetry-1.1.4-py2.py3-none-any.whl (171 kB)
     |████████████████████████████████| 171 kB 2.6 MB/s
INFO: pip is looking at multiple versions of <Python from Requires-Python> to determine which version is compatible with other requirements. This could take a while.
INFO: pip is looking at multiple versions of commitizen to determine which version is compatible with other requirements. This could take a while.
ERROR: Cannot install commitizen==2.11.1 and poetry==1.1.4 because these package versions have conflicting dependencies.

The conflict is caused by:
    commitizen 2.11.1 depends on tomlkit<0.6.0 and >=0.5.3
    poetry 1.1.4 depends on tomlkit<1.0.0 and >=0.7.0

To fix this you could try to:
1. loosen the range of package versions you've specified
2. remove package versions to allow pip attempt to solve the dependency conflict

ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/user_guide/#fixing-conflicting-dependencies```

Copy link
Member

Choose a reason for hiding this comment

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

Then, maybe we should change it to tomlkit = ">=0.7.0,<1.0.0" instead?

Copy link
Contributor Author

@Woklex Woklex Dec 29, 2020

Choose a reason for hiding this comment

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

Then someone might have this situation. What if someone uses commitizen and an x package that has tomlkit < 0.6.0 and >= 0.5.3 in its dependencies? If someone has toolkit < 0.6.0 and >= 0.5.3 then at least we won't have a conflict. And when installing commitzen and poetry, we will install the last package, because poetry tomlkit = >= 0.7.0, < 1.0.0 and commitzen tomlkit = >= 0.5.3, < 1.0.0

Copy link
Member

Choose a reason for hiding this comment

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

If someone has tomlkit < 0.6.0 and >= 0.5.3 then updating to a newer version is cheap, as there are no crazy breaking changes in the changelog and it's recommended to update.

But I'm okay with the proposed change or with the one from Lee tomlkit = ">=0.7.0,<1.0.0", using 0.7.0 would make it more in compliance with toml format. Which one do you prefer?

Regarding the conflicts, poetry is usually not recommended to install it on project level.

And for commitizen, if there were more reports of conflicts, we should consider using a _vendor/ folder, using something like vendy. But so far we haven't had any issues, and we try to keep deps small.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will be satisfied with both options :) I originally wanted to make a toolkit = ">=0.7.0,<1.0.0, but I thought that such changes could break someone's build. So I leave the choice to you. If you prefer >=0.7.0,<1.0.0 then write to me and I will correct the commit.

Copy link
Member

@Lee-W Lee-W Dec 30, 2020

Choose a reason for hiding this comment

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

Ah, I got your point. I misunderstood the meaning of "^0.5.3". I think your solution is a better one. I'll merge it. Thanks for your contribution!

As for dependency conflict, I'd suggest using tools like pipx to install python cli-tools like poetry and commitizen. Maybe we could add it to the doc. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! It is obvious to me that pipx is needed, but I just ran into this problem because another developer did not know it. So it would be ideal to update the documentation and recommend pipx (do as in the poetry documentation). It should be easy to update the documentation and I would even do a PR, but my English is not so good :(

@Lee-W Lee-W merged commit 155f23c into commitizen-tools:master Dec 30, 2020
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