Skip to content

Conversation

yoff
Copy link
Contributor

@yoff yoff commented May 13, 2020

This PR just adds a test to highlight the problem. I will later add a commit to it to also fix the problem.

@yoff yoff requested a review from a team as a code owner May 13, 2020 05:45
@yoff yoff changed the title Test: __bool__ does not raise TypeError by default Python: __bool__ does not raise TypeError by default May 13, 2020
@yoff yoff added the Python label May 13, 2020
Copy link
Member

@RasmusWL RasmusWL 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 a little more context for why __bool__ is a special case is very helpful. I will gladly admit I didn't know the default behavior of __bool__, and had to read the FP report to figure out why we needed this change. I have given a suggestion for an explanatory comment, but feel free to formulate it differently 😉

A general approach I find very useful when fixing FPs is to use a 2-step process:

  1. add FP triggering code to a test-file and update .expected results of the qltest
  2. fix the FP in the query and (again) update the .expected results of the qltest

In this way it's very easy to see what the FP-alert was, just by reviewing the commits one-by-one (although the .expected file doesn't change when viewing the diff of all commits). No need to change this PR, but hopefully this can be a useful method to keep in mind for the future 😊

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
@yoff
Copy link
Contributor Author

yoff commented May 14, 2020

Thanks, just to make sure I understand: Between step 1 and 2, the .expected-file does not actually reflect what we know of the world? That feels a bit wrong :-) Is it not possible to see the intermediate failings?

@yoff
Copy link
Contributor Author

yoff commented May 14, 2020

Ah, Taus has explained this in more detail in my onboarding issue.

Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

What would happen if we now raise a wrong exception for __bool__ ? Before, we would get an alert saying we didn't need to implement it at all (which was wrong), but from reading the code, I don't see how the following code would give any alert.

class Foo(object):
    def __bool__(self):
        raise ZeroDivisionError()

@yoff
Copy link
Contributor Author

yoff commented May 14, 2020

That is a good point, I suppose that should be considered part of solving the problem :-)

@yoff
Copy link
Contributor Author

yoff commented May 14, 2020

Ah, now I see why the merge was needed. It is because I committed your suggested change. I should've just pulled right after that.

@yoff
Copy link
Contributor Author

yoff commented May 15, 2020

redo failed

@yoff
Copy link
Contributor Author

yoff commented May 15, 2020

Check failed due to incorrect line numbers. I must have not run the tests after changing comments or something.

yoff added 2 commits May 15, 2020 13:25
Also update test expectations with changed line numbers.
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

Nice, very good job 👍

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

Successfully merging this pull request may close these issues.

3 participants