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

Fix click v7 version_option compatibility #1410

Merged
merged 1 commit into from Jun 9, 2021
Merged

Fix click v7 version_option compatibility #1410

merged 1 commit into from Jun 9, 2021

Conversation

FuegoFro
Copy link
Contributor

@FuegoFro FuegoFro commented Jun 8, 2021

It looks like the ternary was committed backwards in #1400, since the package_name option was add in click 8, but is only being passed in click 7. That caused pip-tools to error on import when installed alongside click 7.

Changelog-friendly one-liner: Follow up #1400 to fix click v7 compatibility.

Contributor checklist
  • Provided the tests for the changes.
  • Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@FuegoFro
Copy link
Contributor Author

FuegoFro commented Jun 8, 2021

Hey there! I wasn't sure how to set a milestone or what milestone should even be set (possibly 6.1.1, just since that's the milestone which was set on #1400?). Would love some help with that!

Also, I wasn't sure whether I should add a new tox env to tests this, let me know if you think that'd be valuable!

@nicoa nicoa added this to the 6.1.1 milestone Jun 8, 2021
@nicoa
Copy link
Contributor

nicoa commented Jun 8, 2021

you're totally right, good catch! As far as I know, only members of the jazzband organisation can set milestones, I added 6.1.1 for now, agreeing that this makes sense.

Regarding the tox env: I honestly don't know whether it's overkill or highly appreciated to test different click versions, maybe others have a strong opinion on this.

@codecov
Copy link

codecov bot commented Jun 8, 2021

Codecov Report

Merging #1410 (0d6e942) into master (6ed9301) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1410   +/-   ##
=======================================
  Coverage   99.67%   99.67%           
=======================================
  Files          34       34           
  Lines        3042     3042           
  Branches      327      327           
=======================================
  Hits         3032     3032           
  Misses          5        5           
  Partials        5        5           
Impacted Files Coverage Δ
piptools/scripts/compile.py 100.00% <100.00%> (ø)
piptools/scripts/sync.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 6ed9301...0d6e942. Read the comment docs.

@webknjaz
Copy link
Member

webknjaz commented Jun 8, 2021

This will probably need to be synced with master to unblock the merge button.

It looks like the ternary was committed backwards in #1400, since the `package_name` option was add in click 8, but is only being passed in click 7. That caused `pip-tools` to error on import when installed alongside click 7.
@FuegoFro
Copy link
Contributor Author

FuegoFro commented Jun 8, 2021

Thank you for the quick review! 😀 I've gone ahead and rebased the branch on latest master 🙂

@ssbarnea ssbarnea merged commit 32a8834 into jazzband:master Jun 9, 2021
@FuegoFro FuegoFro deleted the click_v7_compat branch June 10, 2021 20:22
@atugushev atugushev changed the title Fix click v7 version_option compatibility Fix click v7 version_option compatibility Jun 21, 2021
@atugushev atugushev modified the milestones: 6.1.1, 6.2.0 Jun 21, 2021
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

5 participants