Skip to content

Switch monkeypatch fixture to yield syntax #2173

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

Merged
merged 1 commit into from
Jan 5, 2017
Merged

Switch monkeypatch fixture to yield syntax #2173

merged 1 commit into from
Jan 5, 2017

Conversation

jeffwidman
Copy link
Contributor

@jeffwidman jeffwidman commented Jan 4, 2017

If you're open to merging this, let me know if I should target a different branch than master and I'll amend the commit to include updates to changelog/authors

@nicoddemus
Copy link
Member

nicoddemus commented Jan 4, 2017

Hi @jeffwidman,

Is there a reason for this other than just improving the internal code a bit? If there are no external changes I don't think we should add a CHANGELOG entry as this won't affect users.

Otherwise it is a welcome mini-improvement IMO. 😁

request.addfinalizer(mpatch.undo)
return mpatch
yield mpatch
mpatch.undo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be .undo() as we actually want to call the method now, not only pass a callable.

@jeffwidman
Copy link
Contributor Author

Purely internal code cleanup. Agreed there's no point in changelog. Fixed and force-pushed

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.816% when pulling 49fd87c on jeffwidman:patch-1 into b769e41 on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.816% when pulling 6d81c68 on jeffwidman:patch-1 into b769e41 on pytest-dev:master.

@nicoddemus
Copy link
Member

Thanks @jeffwidman, appreciate it!

@nicoddemus nicoddemus merged commit 9477f59 into pytest-dev:master Jan 5, 2017
@jeffwidman jeffwidman deleted the patch-1 branch January 5, 2017 05:32
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.

4 participants