consolidate vcs link detection #6883
Conversation
sbidoul
force-pushed
the
is_vcs-refactor-sbi
branch
from
0a69e0a
to
7684037
Compare
src/pip/_internal/models/link.py
Outdated
| def is_vcs(self): | ||
| from pip._internal.vcs import vcs | ||
|
|
||
| return self.scheme in vcs.all_schemes |
There was a problem hiding this comment.
Since with this change this logic is getting duplicated in both is_vcs and is_artifact, I think it would be best if is_artifact is changed to call this method. (And you can move is_vcs before is_artifact so is_artifact is depending on methods defined before it.) Also, that way we avoid introducing a second nested import.
|
Looks good to me. |
src/pip/_internal/models/link.py
Outdated
| from pip._internal.vcs import vcs | ||
|
|
||
| if self.scheme in vcs.all_schemes: | ||
| if self.is_vcs: |
There was a problem hiding this comment.
Actually, this can just be:
return not self.is_vcsThere was a problem hiding this comment.
Yes of course, but reading the docstring I had the feeling it was meant to evolve to cover other cases. That is also the reason I refrained of implementing is_vcs as not is_artifact. What do you think?
There was a problem hiding this comment.
It's always possible things can change in the future, but we can cross that bridge when we get to it! :)
There was a problem hiding this comment.
FWIW, I do agree the current docstring is confusing..
There was a problem hiding this comment.
Okay, done and squashed.
sbidoul
force-pushed
the
is_vcs-refactor-sbi
branch
from
fe32b88
to
2e1dfbe
Compare
lock
bot
added
the
auto-locked
While working on #6853 and #6851, it was not immediately obvious to me why
not link.is_artifactmeans it is a VCS link.So I thought I could make this little readability improvement, by replacing the global
is_vcs_urlfunction by ais_vcsproperty ofLink.