[wip] Fix ignore_path condition #2475
Conversation
ant31
changed the title
|
Hi, |
|
Thanks for the PR. Could you please explain the issue this solves in detail? It is not entirely obvious at first glance... |
|
@nicoddemus Sorry, I was doing it, (see PR description). Let me know if you need more details |
ant31
changed the title
|
Thanks @ant31 that's clear now |
| @@ -175,7 +175,7 @@ def pytest_ignore_collect(path, config): | |||
| if excludeopt: | |||
| ignore_paths.extend([py.path.local(x) for x in excludeopt]) | |||
|
|
|||
| if path in ignore_paths: | |||
| if py.path.local(path) in ignore_paths: | |||
There was a problem hiding this comment.
Strange, line 171 is p = path.dirpath(), which suggests path is already a py.path.local instance...
There was a problem hiding this comment.
Gentle ping to @ant31. 😁
I'm asking because I would like to fully understand the patch before it being accepted.
There was a problem hiding this comment.
p is a path.dirpath(), but not path.
I removed the local variable p to make it clearer (https://github.com/pytest-dev/pytest/pull/2475/files#diff-5b415004cf343fc3c36aebb8aad93882R171)
There was a problem hiding this comment.
But the return value of dirpath should be a path ?!?
There was a problem hiding this comment.
@RonnyPfannschmidt / @nicoddemus
You are right, it's already a py.path.local
It still fixes my issue because applying twice (?) it strips the leading /:
(Pdb) path
local('///digest')
(Pdb) py.path.local(path)
local('/digest')
not sure where to fix that now
There was a problem hiding this comment.
We can do both to be honest, because it is simple to workaround it here for users which don't have the same py version.
There was a problem hiding this comment.
pytest-dev/py#128 has been merged, should we merge this one as well @RonnyPfannschmidt?
There was a problem hiding this comment.
Oh I just realized we probably need a newsfragment for this as well.
There was a problem hiding this comment.
@nicoddemus we should merge this one as well after a news fragment is made
There was a problem hiding this comment.
Added a news fragment file. 👍
|
Thanks @ant31! |
|
@nicoddemus @RonnyPfannschmidt Awesome, appreciate how fast you fixed it. (btw, great job on pytest, we're loving it) |
For some legacy reason, I have a container copying source to '/' directly.
From there
rootdir='/'and paths gathered by pytest have 3 leading /.the condition https://github.com/pytest-dev/pytest/pull/2475/files#diff-5b415004cf343fc3c36aebb8aad93882L178
is checking the following and it doesn't match:
This pr simply format the path the way the excludepath is formatted
Thanks for submitting a PR, your contribution is really appreciated!
Here's a quick checklist that should be present in PRs:
$issue_id.$typefor example (588.bug)removal,feature,bugfix,vendor,docortrivialbugfix,vendor,docortrivialfixes, targetmaster; for removals or features targetfeatures;Unless your change is a trivial or a documentation fix (e.g., a typo or reword of a small section) please:
AUTHORS;