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.

Sign up for GitHub

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

Jump to bottom

Revert "A warning is now issued when assertions are made for None" #6234

Merged
merged 1 commit into from Nov 26, 2019
Merged

Revert "A warning is now issued when assertions are made for None" #6234

merged 1 commit into from Nov 26, 2019

Conversation

asottile
Copy link
Member

Resolves #4639

CC @Tadaboody

blueyed
blueyed reviewed Nov 20, 2019
View reviewed changes
Copy link
Contributor

@blueyed blueyed left a comment •
edited

Choose a reason for hiding this comment

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

I think it is a good feature in general, and should therefore get fixed / improved instead.

If being reverted there should be a test for the new behavior then at least.

But I think we should decide what to do in #4639 first.

testing/test_warnings.py
@@ -543,7 +543,7 @@ def assert_result_warns(result, msg):

def test_tuple_warning(self, testdir):
testdir.makepyfile(
"""
"""\
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not change code unnecessary, especially when it is only to please your editor in some way (IIRC).
For all others this adds visual disbalance, and often gets highlighted in a special way (line continuation).

Copy link
Contributor

Choose a reason for hiding this comment

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

?

nicoddemus
nicoddemus approved these changes Nov 20, 2019
View reviewed changes
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

I think this is good to go as is, don't see why we would need a test for it. 👍

@nicoddemus
Copy link
Member

This can go to master btw (it can't possibly break anything).

@asottile asottile changed the base branch from features to master November 20, 2019 16:21
@asottile
Copy link
Member Author

rebased on master!

@Tadaboody
Copy link
Contributor

Oof another one bites the dust then (honestly it’s pretty satisfying seeing the revert being this smooth).

Is it worth it to try and eliminate the false positive? Maybe try and only warn if it fails the test? (Some kinda try wrapping it)

@nicoddemus
Copy link
Member

@Tadaboody I don't think so, most cases the test will at least fail with a message anyway, as @asottile pointed out.

@blueyed anything else here?

@blueyed
Copy link
Contributor

blueyed commented Nov 21, 2019

@blueyed anything else here?

I do not like the (unnecessary) code style change (and being ignored for pointing it out (across a rebase where it could have been fixed)), and will likely investigate in what I've said in #4639 (comment).
I am not blocking this though, but will likely carry this into my fork then also.. 👍

@asottile asottile merged commit 209d991 into pytest-dev:master Nov 26, 2019
@asottile asottile deleted the remove_none_warning branch November 26, 2019 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blueyed blueyed blueyed left review comments

@nicoddemus nicoddemus nicoddemus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warning about assert None from XFAIL tests
4 participants
@asottile @nicoddemus @Tadaboody @blueyed