Skip to content
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

plugin: Use pytest 5.4.0 new Function API #142

Merged
merged 1 commit into from Mar 31, 2020
Merged

plugin: Use pytest 5.4.0 new Function API #142

merged 1 commit into from Mar 31, 2020

Conversation

romainletendart
Copy link

This fixes #141.

@mjpieters
Copy link
Contributor

This change means this project can only work with Pytest 5.4.0 and up. Either

  • Update the install_requires metadata to reflect this dependency
  • Allow for a fallback, perhaps with a try: ... except AttributeError: block, to calling pytest.Function() directly again.

@romainletendart
Copy link
Author

@mjpieters Thank you for your review.
I have updated install_requires accordingly.

@simonfagerholm
Copy link
Contributor

@romainletendart: The reason this PR fails has been fixed in #148, just waiting for review (maybe @mjpieters has the time to check?) and merge.
This branch will probably need to pull in the master branch when that happens, as the changes are needed and that my commit also changed the install_requires, but to only work with pytest < 5.4.0.

@dmitrypolo
Copy link

@simonfagerholm wouldn't it be better to follow @mjpieters advice and instead of constraining users from using newer versions of pytest to have a fallback to call pytest.Function() based on the version?

@simonfagerholm
Copy link
Contributor

@dmitrypolo maybe you meant to message @romainletendart?

@dmitrypolo
Copy link

@simonfagerholm @romainletendart my comment applies to both of you technically. This PR confines users from using pytest < 5.4 whereas yours confines users from using pytest > 5.4.

@simonfagerholm
Copy link
Contributor

@dmitrypolo Before this PR is merged pytest 5.4.0 and above is not supported. To be able to use pytest >= 5.4.0 this PR is needed. My PR correctly reflects that only pytest < 5.4.0 is supported

@dmitrypolo
Copy link

@simonfagerholm correct, I guess the point I am trying to get across is we should probably try to allow for both users of pytest < 5.4 and pytest > 5.4 to use this plug-in. The issue is that both of these PRs restrict users in one-way or the other.

@simonfagerholm
Copy link
Contributor

@dmitrypolo Yes but in my case its for good reason: It isn't compatible... I can understand that you would like it to be backwards compatible, but that is only with this PR and not mine.

@romainletendart
Copy link
Author

@simonfagerholm wouldn't it be better to follow @mjpieters advice and instead of constraining users from using newer versions of pytest to have a fallback to call pytest.Function() based on the version?

I'm not sure supporting multiple API versions inside the code itself is a good idea.
Just suppose you do this for every API change and your code quickly becomes unreadable because of all the ifs you added to support multiple versions.
Plus, you get high chances to get corner case bugs with some weird combinations of versions that were not supposed to work together initially.

@simonfagerholm, I will definitely rebase on the upstream master once your merged your PR just to be sure everything is green before being able to merge my own PR.

@simonfagerholm
Copy link
Contributor

@romainletendart I totally agree.
I'll let you know when its merged. Just need someone with write access to review and approve.

@romainletendart
Copy link
Author

Maybe we could ask @Tinche as he seems to be both the initiator and the main contributor of pytest-asyncio?

@Tinche
Copy link
Member

Tinche commented Mar 29, 2020

@romainletendart Fix the merge conflict please and let's get this in!

@simonfagerholm
Copy link
Contributor

@romainletendart Like @Tinche said: my PR is in, pull it and lets get this in! :)

@romainletendart
Copy link
Author

@Tinche @simonfagerholm Merge conflict fixed, thank you! :)

@simonfagerholm
Copy link
Contributor

@Tinche Seems good to go! :)

@Tinche Tinche merged commit 6397a22 into pytest-dev:master Mar 31, 2020
@Tinche
Copy link
Member

Tinche commented Mar 31, 2020

Thank you kindly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pytest 5.4.0 deprecated pytest.Function() construction
5 participants