-
-
Notifications
You must be signed in to change notification settings - Fork 612
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
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
Changes from 6 commits
6b81eeb
0b07690
d46cf97
1d70557
fbef894
726f819
462fb9c
bc2034f
77af4d2
3478be3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
from .click_compat import IS_CLICK_VER_8_PLUS | ||
from .pip_compat import PIP_VERSION, parse_requirements | ||
|
||
__all__ = ["PIP_VERSION", "parse_requirements"] | ||
__all__ = ["PIP_VERSION", "IS_CLICK_VER_8_PLUS", "parse_requirements"] | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import click | ||
nicoa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
from pip._vendor.packaging.version import Version | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, was just in analogy to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that should be enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although, you may want to multi-line it to keep the inline complexity low There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nvmd, will simply add a comment then in between the brackeets There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
|
||
CLICK_VERSION = Version(click.__version__).major | ||
nicoa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
IS_CLICK_VER_8_PLUS = CLICK_VERSION > 7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at least pycodestyle seems to do so, compare PyCQA/pydocstyle#62