Fix handling of ini configuration in subdirs when specifying :: #2149
Conversation
pelme
changed the title
|
|
||
| * Specifying tests with colons like ``test_foo.py::test_bar`` for tests in | ||
| subdirectories with ini configuration files now uses the correct ini file. | ||
| Thanks `@pelme`. |
There was a problem hiding this comment.
Missing link for user name and issue link as well?
There was a problem hiding this comment.
Thanks, I've updated the PR!
| if not str(arg).startswith('-') | ||
| ] | ||
|
|
||
| return [d for d in possible_dirs if d.exists()] |
There was a problem hiding this comment.
Is it harder to parse? Maybe ...
return [
py.path.local(str(arg).split('::')[0])
for arg in args
if not str(arg).startswith('-') and arg.exists()
]There was a problem hiding this comment.
I played around with a couple of variants of this. I was not a big fan of the original nested generator expression in the list comprehension and it became worse with this additional check.
The args may not be py.path.local from the beginning. Also, not stripping the part after :: before calling .exists() leaves us with the original bug. To do everything in a single list comprehension it would then look like
return [
py.path.local(str(arg).split('::')[0])
for arg in args
if not str(arg).startswith('-') and py.path.local(str(arg).split('::')[0]).exists()
]I was not happy with that since stripping :: and creating the py.path.local object happens twice.
There was a problem hiding this comment.
Why don't you like my generator expression?! ;)
In your case str is still called twice on arg.. but I can see that readability is better with a named var here.
Nitpick: remove the added newline to keep it more compact.
There was a problem hiding this comment.
I agree using a generator expression instead of a list comprehension would improve things a bit, although I'm pretty sure this won't impact performance at all. 😉
There was a problem hiding this comment.
IMHO what would improve readability would be to use small functions that act as documentation to explain all that's being handled there:
def get_dirs_from_args(args):
def is_option(x):
return str(x).startswith('-')
def get_file_part_from_node_id(x):
return str(x).split('::')[0]
possible_dirs = (
py.path.local(get_file_part_from_node_id(arg))
for arg in args
if not is_option(arg)
)
return [d for d in possible_dirs if d.exists()]Also I just noticed that although the function is named get_dirs_from_args, it actually returns directories and filenames. 😁
There was a problem hiding this comment.
I've updated the PR to use a generator expression, small functions and made it always return dirs for consistency. I was also able to get rid of a couple of redundant checks in get_common_ancestor.
| if not str(arg).startswith('-') | ||
| ] | ||
|
|
||
| return [d for d in possible_dirs if d.exists()] |
There was a problem hiding this comment.
Why don't you like my generator expression?! ;)
In your case str is still called twice on arg.. but I can see that readability is better with a named var here.
Nitpick: remove the added newline to keep it more compact.
|
🍻 |
| return path | ||
| return py.path.local(path.dirname) | ||
|
|
||
| # These looks like paths but may not exist |
pelme
force-pushed
the
issue2148
branch
2 times, most recently
from
8be42a3 to
b692c64
Compare
…ains ::. This commit also improves readbility in get_dirs_from_args by using self documenting local functions. get_dirs_from_args also now only returns directories that actually exists, and not files to avoid confusion. This commit also removes redundant checks in get_common_ancestor that was already performed in get_dirs_from_args..
This PR fixes #2148.