-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix handling of duplicate args with regard to Python packages #4319
Fix handling of duplicate args with regard to Python packages #4319
Conversation
0d7525e
to
3c595f0
Compare
Found a fix. This was introduced with e041823, but before it would only collect 1 item:
|
3c595f0
to
9a450d7
Compare
Sorry, it does not fix #4310 yet. |
9a450d7
to
fa35f65
Compare
Reverted _collect_seen_pkgdirs which was meant to collect packages first, but |
Definitely agree that we need to improve collection tests coverage and refactor the code itself. |
Ahem.. this was not really ready for merging - there is e.g. a TODO commit in there.. :/ Sorry for not marking it WIP, but it is clearly visible from the commit history.. |
ouch my bad, i did read the commit lines but i completely missed the |
..and the XXX. Typically I would like to get fixes / refactors like this into a single commit (not even the annoying merge commits), i.e. squash-merge them when ready. It is really frustrating to go back and forth with all the single commits that have been done in this area already for the package-scoped fixtures.. :( |
..and everything in this regard seems to be ad-hoc, instead of discussing/reviewing stuff carefully. |
] | ||
) | ||
result = testdir.runpytest("./tests/test_foo.py", "--collect-only") | ||
result.stdout.fnmatch_lines(["*<Module 'test_foo.py'>", "*<Function 'test_foo'>"]) | ||
result.stdout.fnmatch_lines( | ||
["<Package */tests'>", " <Module 'test_foo.py'>", " <Function 'test_foo'>"] |
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.
btw: can black be told to keep these as separate lines?
It is much more readable then IMHO.
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.
@blueyed you can use # fmt: off/on
blocks for those, i believe we also support multiline strings that will be split/striped
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.
AFAIK black doesn't have many configuration options, on purpose.
Packages are collected twice.
Fails with:
Found while looking into #4310.