-
Notifications
You must be signed in to change notification settings - Fork 9
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 warnings from pytest 3.8 #3
Conversation
On python 3.5 this failed with 'no such option: --use-wheel'.
3.7 is not yet supported by travis.
`--source` takes a module name, not the file name.
This is implicitly supported by the old pytest mark behaviour in which args and kwargs for multiple marks are merged.
That last travis build hung before even executing anything in the job... It's probably fine now. |
Hey, thanks for the effort! :-) I'll have a look as soon as I can. |
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.
Just some minor comments. In general it looks great. I appreciate you splitting the change into small commits with good commit messages. Commit messages are one of my favourite pet peeves ;-)
on_duplicate = options['on_duplicate'] | ||
keep_top_dir = options['keep_top_dir'] | ||
|
||
if not content: | ||
return tmpdir |
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.
I think it would be a little better to have these two lines above line 77 (on_duplicate = ...) because returning as soon as possible is clearer. There is no reason to process the options if no content exists.
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.
Sure. I intended to separate the mark parsing and the logic, but either is fine by me.
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.
You did not fix this... but anyway it was not very important.
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.
Sorry, I mis-interpreted the meaning of your thumbs-up. Thanks for merging anyway...
setup.py
Outdated
@@ -13,7 +13,7 @@ def _read(fname): | |||
|
|||
DEPENDENCIES = [ | |||
'py', | |||
'pytest', | |||
'pytest~=3.6', |
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.
I'm not aware of this syntax and even if it is allowed if think this is more obvious and therefore better:
pytest>=3.6
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.
It's documented here:
https://www.python.org/dev/peps/pep-0440/#compatible-release
and declares that it requires a pytest release that's compatible with 3.6 (i.e. before 4.x); would you prefer pytest == 3.*, >= 3.6
?
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.
Thanks for the link.
I think >= 3.6 is best. In general I prefer to be permissive with the versions unless I know there is an issue. People should be encouraged not prevented from upgrading.
As a side-note: To ensure stable and reproducible environments I would always recommend exactly pinning your versions (e.g. using pip-tools or Pipenv) and trying to upgrade those versions in regular intervals.
Process each mark separately, rather than relying on automatic merging of args and kwargs. See https://docs.pytest.org/en/latest/mark.html#updating-code This requires pytest 3.6, which does not run on python 2.6. Travis environments have pytest 3.4 installed by default; this needs to be explicitly updated. Fixes omarkohl#2
This was sometimes failing with a long traceback including `AttributeError: _DistInfoDistribution__dep_map` when trying to upgrade pytest, which can apparently be solved by upgrading pip. We'll see.
There were two duplicate files (file1 and file4), and the exception raised could reference either depending on the filesystem.
dba46d0
to
ade09e0
Compare
Hi, sorry about the delay. I've changed the version requirement as you suggested. I also found and fixed a bug in the tests that was causing them to fail on one machine but not another; see the latest commit. I'd normally open another PR but there's already a bunch of unrelated commits in here... Thanks! |
This fixes #2.
I had to make a few other changes to fix up the build system, and added a test for some implicit behaviour of the existing code.
This version is not compatible with pytest versions before 3.6, which means it wont run on python 2.6. IMO this is a good thing, as python 2.6 is no longer receiving security patches. If you would rather keep compatibility then there's a commit that does that on my
misc_fixes_compat
branch.