Pass `package_name` explicitly in `click.version_option` decorators for compatibility with `click>=8.0` #1400

nicoa · 2021-05-19T12:07:50Z

Closes #1398 .

Unfortunately mypy threw some errors during pre-commit run, where two of them seem to be valid (coming from CI as well).

Further, some of the tests failed, but this seems to be related to a pip.conf file adding extra indices to the compiled requirements, I'll re-run them again to see if this is related.

Changelog-friendly one-liner: pass package_name explicitly in click.version_option decorators for compatibility with click>=8.0

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).

codecov · 2021-05-19T12:33:45Z

Codecov Report

Merging #1400 (3478be3) into master (16ff084) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1400   +/-   ##
=======================================
  Coverage   99.67%   99.67%           
=======================================
  Files          34       34           
  Lines        3039     3041    +2     
  Branches      327      327           
=======================================
+ Hits         3029     3031    +2     
  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 16ff084...3478be3. Read the comment docs.

nicoa · 2021-05-19T14:29:02Z

the failing mypy CI is due to click not being upgraded yet in typeshed. What do you think, should I add a # type: ignore to the two respective lines or wait for typeshed to incorporate click 8 changes?

nicoa · 2021-05-19T14:43:06Z

Update: see python/typeshed#5503, this seems to be an issue with click directly. same questions still applies: ignore that typing or report to click. It seems that mypy is still catching up the old typeshed stubs instead of the new typing click8.0 provides.

davidism · 2021-05-20T02:10:45Z

Click 8 provides typing directly, typeshed stubs are not used anymore. Not sure how the pre-commit mypy hook works, are you sure you're getting the latest click version in that environment? (By "you" I mean a pip-tools maintainer, not sure if you are one.)

nicoa · 2021-05-20T15:55:33Z

Hey @davidism , no I'm not a maintainer, just wanted to contribute something here :) I can reproduce the issue with pre-commit.ci locally, but the corresponding virtualenv mypy is using seems to use click 8, so I have no idea why the click typing is not used.

Would definitely appreciate the support of some maintainer here debugging the issue, maybe this is something more trivial than it looks to me.

nicoa · 2021-05-26T11:09:35Z

Hey, are any maintainers around here and reading this? It seems that the pre-commit CI is optional for merging, would be great to get some comment on whether it's preferred to first merge that or first fix the failing CI job.

Regarding the mypy hook (the failing one): it seems that additional_dependencies: [click>=8.0.0] in pre-commit-config fixes the wrong click version, but introduces a whole lot of new errors (probably due to switch to new click version, not sure). I can provide them here if required. Would it make sense to open a ticket to fix them?

atugushev · 2021-05-26T11:12:57Z

Regarding the mypy hook (the failing one): it seems that additional_dependencies: [click>=8.0.0]

Aw, that makes sense now, since we cache installed dependencies in CI. @webknjaz Is there a decent way to drop the cache (other than update the cache key or example)?

nicoa · 2021-05-26T11:14:13Z

Still, there would be other errors introduced then with the new click version: I'd open a separate ticket for that, would you agree with that procedure?

atugushev · 2021-05-26T11:17:17Z

Still, there would be other errors introduced then with the new click version: I'd open a separate ticket for that, would you agree with that procedure?

Yeah, let's track this in a separate issue. Thanks! In addition to click, I'm inclined to think that the latest pip update brought new types and we need to address that also.

webknjaz · 2021-05-26T11:42:00Z

Is there a decent way to drop the cache (other than update the cache key or example)?

Not that I know of. Although, you seem to be assuming the wrong check. The one that is failing is not coming from GHA. It comes from the pre-commit.ci service.

nicoa · 2021-05-26T11:50:07Z

The newest pre-commit ci run seemed to use the proper version and raised the same errors I experienced when explicitly adding the new dependency (compare #1401 and #1402 ). I'd say this can be merged while ignoring the failing pre-commit.ci check, what do you think @webknjaz ?

Well, just realized that it would make sense to then upgrade install_requires to click >= 8 as well, as the option was introduced there. Anything speaking against that?

webknjaz · 2021-05-26T12:10:57Z

Yes, unless we want to have broad compatibility with older click, we should bump the dep too.

Regarding broken typing, if it's directly connected to this change, it must be fixed as a part of it.

nicoa · 2021-05-26T13:52:19Z

will bump the dependency then.

Regarding the typing: no, it's due to new click and is on latest pre-commit-CI as well, this just didn't had it due to some caching apparently. Would be solved in #1402 .

webknjaz · 2021-05-26T14:22:43Z

But bumping click is directly related to this then. This PR cannot be accepted with breaking typing.

nicoa · 2021-05-26T15:12:37Z

Sorry, that was a typo... I wanted to write that it's due to new pip version - so not related in any way to click and the current CI setup is broken with the same errors if running now as well already (as you can see in #1401 ).

webknjaz · 2021-05-26T20:22:36Z

Oh, I misunderstood that. Thinking more about dropping the older click, I think it's probably preferable to support those. This is because click is fairly popular and there's a good chance that some users will have other tools in the same env that may pin click more strictly and so it's a potentially serious conflict we'd cause.

piptools/scripts/compile.py

nicoa · 2021-05-27T07:12:33Z

fair point, but not sure how to handle as the current setup is broken sometimes under click 8. Made some inline suggestion in the code to keep click 7 but handle 8 separately then.

webknjaz · 2021-05-27T07:55:59Z

There's a few ways:
1)

ver_kwargs = {} if click7 else {'package_name': 'piptools'} 
...
@click.version_option(**ver_kwargs)
def cli(...): ...

...
def cli(...): ...

if click8:
    cli = click.version_option(package_name='piptools')(cli)

from itertools import partial
ver_opt = click.version_option if click7 else partial(click.version_option, package_name='piptools')
...
@ver_opt()
def cli(...): ...

nicoa · 2021-05-27T08:29:40Z

That's what I suggested, sorry if that was not clear :-) was just waiting for the okay to do this version-based differencing, would implement option 1 then. Thanks!

…the version of click

piptools/scripts/compile.py

…ersion_option arguments to click

for more information, see https://pre-commit.ci

webknjaz · 2021-05-27T15:10:07Z

piptools/_compat/__init__.py

 from .pip_compat import PIP_VERSION, parse_requirements

-__all__ = ["PIP_VERSION", "parse_requirements"]
+__all__ = ["PIP_VERSION", "IS_CLICK_VER_8_PLUS", "parse_requirements"]


Side note: why is this a list? It's usually recommended to make __all__ immutable, hence use a tuple.

P.S. This is out of the scope of this PR, just wanted to make a note.

to be fair, this is a list as well in the python tutorial on modules: https://docs.python.org/3/tutorial/modules.html#importing-from-a-package and the pip package has this format as well.
Do you want to change this here or should I leave as it is? I actually don't have any preference here.

No, don't touch this because it's out of the scope. But in the future, this should probably be added. I've added this comment for history.

P.S. Normally, pylint would warn about this problem but I don't think it's integrated.

at least pycodestyle seems to do so, compare PyCQA/pydocstyle#62

piptools/_compat/click_compat.py

webknjaz · 2021-05-27T15:40:08Z

piptools/_compat/click_compat.py

@@ -0,0 +1,5 @@
+import click
+from pip._vendor.packaging.version import Version


I don't really like relying on how and whether pip vendors its deps. Plus it may cause problems when repackaged in distros that unbundle vendored software.

sure, was just in analogy to pip_compat module - would you agree with using int(click.__version__.split(".")[0])?

Yeah, that should be enough.

Although, you may want to multi-line it to keep the inline complexity low

not sure what you mean with multi-lining, as for me black would merge this into one line if not forcing to not do on the following:

int( click.__version__.split(".")[0] )

nvmd, will simply add a comment then in between the brackeets

Yeah, that's one of the reasons I dislike black...

As for the line complexity, I like this metric that https://wemake-python-stylegui.de introduced called Jones complexity. It really makes a difference maintainability/readability-wise!

Here's some more info on this https://sobolevn.me/2019/10/complexity-waterfall#lines.

I wish this set of linter plugins was integrated here.

didn't knew the blog entry regarding the complexity until now, found it a good read and would recommend to colleagues, thanks a lot for that, very interesting!

piptools/_compat/click_compat.py

nicoa · 2021-05-27T20:42:38Z

Hey @webknjaz thanks for your patience with me during this PR! For me it looks like it is ready to get into a next release. Whats the progress from here? would you merge it to master directly and/or add a milestone (I am not able to do any of those two)? And do I have to do anything from my side or does everything look good now?

Update: ... at least regarding the milestone I made it :)

webknjaz · 2021-05-27T21:47:36Z

So it looks like the CI is failing because of an unrelated problem so that would need to be fixed before merging this PR.

Regarding the release, I don't do those so you'll have to wait.

nicoa · 2021-05-27T21:55:06Z

Alright, would wait then, thanks for the information! Yes, for the CI there is issue #1403 .

nicoa · 2021-06-01T16:09:25Z

Hey @webknjaz , as you already reviewed this I'd merge it, but I'm not sure if this is only fine for more senior persons on the project. Should I proceed or do you / others want to keep the hands on what gets to the main branch?

webknjaz · 2021-06-04T15:59:16Z

@nicoa feel free to merge

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.

It looks like the ternary was committed backwards in jazzband/pip-tools#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.

pass package_name explicitly in click.version_option decorators

6b81eeb

nicoa mentioned this pull request May 19, 2021

add package_name to click.version_option stub python/typeshed#5503

Closed

nicoa mentioned this pull request May 26, 2021

pre-commit CI is failing #1402

Closed

bump dependency to version click >= 8

0b07690

nicoa commented May 27, 2021

View reviewed changes

piptools/scripts/compile.py Outdated Show resolved Hide resolved

nicoa added 2 commits May 27, 2021 11:14

reenable click7 and provide package options depending on a switch on …

d46cf97

…the version of click

remove unnecessary split string in click version comparison

1d70557

webknjaz reviewed May 27, 2021

View reviewed changes

piptools/scripts/compile.py Outdated Show resolved Hide resolved

nicoa and others added 2 commits May 27, 2021 15:48

added compat module for click version and use that to determine the v…

fbef894

…ersion_option arguments to click

[pre-commit.ci] auto fixes from pre-commit.com hooks

726f819

for more information, see https://pre-commit.ci

webknjaz reviewed May 27, 2021

View reviewed changes

piptools/_compat/click_compat.py Outdated Show resolved Hide resolved

name variable corresponding to click major version properly

462fb9c

webknjaz reviewed May 27, 2021

View reviewed changes

piptools/_compat/click_compat.py Outdated Show resolved Hide resolved

webknjaz approved these changes May 27, 2021

View reviewed changes

get rid of dependency on pip vendored packages

bc2034f

nicoa added this to the 6.2.0 milestone May 27, 2021

Merge branch 'master' into master

77af4d2

Merge branch 'master' into master

3478be3

nicoa merged commit 21bb5ac into jazzband:master Jun 4, 2021

nicoa modified the milestones: 6.2.0, 6.1.1 Jun 4, 2021

FuegoFro mentioned this pull request Jun 8, 2021

Fix click v7 version_option compatibility #1410

Merged

3 tasks

atugushev changed the title ~~pass package_name explicitly in click.version_option decorators~~ Pass package_name explicitly in click.version_option decorators for compatibility with click>=8.0 Jun 21, 2021

atugushev added the bug fix label Jun 21, 2021

atugushev modified the milestones: 6.1.1, 6.2.0 Jun 21, 2021

		@@ -0,0 +1,5 @@
		import click
		from pip._vendor.packaging.version import Version

Pass package_name explicitly in click.version_option decorators for compatibility with click>=8.0 #1400

Pass package_name explicitly in click.version_option decorators for compatibility with click>=8.0 #1400

Conversation

nicoa commented May 19, 2021 • edited

Contributor checklist

codecov bot commented May 19, 2021 • edited

Codecov Report

nicoa commented May 19, 2021

nicoa commented May 19, 2021 • edited

davidism commented May 20, 2021 • edited

nicoa commented May 20, 2021

nicoa commented May 26, 2021

atugushev commented May 26, 2021

nicoa commented May 26, 2021 • edited

atugushev commented May 26, 2021

webknjaz commented May 26, 2021

nicoa commented May 26, 2021 • edited

webknjaz commented May 26, 2021

nicoa commented May 26, 2021

webknjaz commented May 26, 2021

nicoa commented May 26, 2021

webknjaz commented May 26, 2021

nicoa commented May 27, 2021

webknjaz commented May 27, 2021

nicoa commented May 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoa commented May 27, 2021 • edited

webknjaz commented May 27, 2021

nicoa commented May 27, 2021

nicoa commented Jun 1, 2021 • edited

webknjaz commented Jun 4, 2021

Pass `package_name` explicitly in `click.version_option` decorators for compatibility with `click>=8.0` #1400

Pass `package_name` explicitly in `click.version_option` decorators for compatibility with `click>=8.0` #1400

nicoa commented May 19, 2021 •

edited

codecov bot commented May 19, 2021 •

edited

nicoa commented May 19, 2021 •

edited

davidism commented May 20, 2021 •

edited

nicoa commented May 26, 2021 •

edited

nicoa commented May 26, 2021 •

edited

nicoa commented May 27, 2021 •

edited

nicoa commented Jun 1, 2021 •

edited