leap: Change tests assertIs(..., True) for assertTrue(...)

[jgomo3]

Predicate functions shouldn't convert its implementation based on
logic expressions (combinations of not, and and or).

I.e. The following should be correct:

def is_NOT_divisible_by_4(n):
    return n % 4

assert is_NOT_divisible_by_4(10)

But as the TestCases for this exercise are using assertIs(..., True) instead of assertTrue(...)`, implementations cointing on the numbers
behaviour on boolean contexts will Fail.

What this mean, is that for the given definition of
is_NOT_divisible_by_4, the folloging assertion will Fail:

assert is_NOT_divisible_by_4(10) == True

I think it is meaningless to check for True as it is naive to write:

if predicate(x) == True:
    do_something(x)

Instead of:

if predicate(x):
    do_something(x)

[behrtam]

I strongly disagree. But maybe we can get more feedback on this. /cc @rootulp @pheanex @treyhunner

The history is that C back in 1972 didn't have a Boolean type and to this day they are commonly represented by integers in C programs. Also Python started without Boolean and added it with 2.3 if I remember correctly. So, that -2, 1, 42 evaluate to True and only 0 evaluates to False is a relic for compatibility reasons.

A predicate maps every argument to either true or false and a function maps every argument to a value. So perfect examples for a predicate functions like is_NOT_divisible_by_4 or is_leap_year should explicitly return True or False and not put their trust in some old implementation detail.

[pheanex]

I'd go with the PEP8 suggestion: "Don't compare boolean values to True or False using ==".
I personally think it is shorter, more expressive and therefore better readable.
I guess PEP20/The Zen of Python may support this.

[jgomo3]

@behrtam I feel you, but remember that Python defines boolean_contexts. So [], None, False and 0 are false in that context. So:

# Altough ...
if [1,2,3]: print("will be printed")
# This ...
if [1,2,3] == True: print("this will not be printed :'(")
if [1,2,3] is True: print("and this neither :'(")

I've seen this practice not as a baggage from ancient times, but a Python way of reducing code, avoiding stuffs like:

if boolean([1,2,3]): print("will be printed")

Altough good in one way, any one could argue it is bad in the sense that it promotes the if [1,2,3]: instead the more explicit:

if len([1,2,3]) > 0: ...

But in any case, it is just another discussion that does nothing with the fact that in Python not only you can test truthfulness directly from some objects which implement a boolean context, but it is actually encouraged.

[treyhunner]

I'm torn on this.

I encourage students/peers to rely on truthiness to test for emptiness.

numbers = [1, 2, 3]

if numbers:
    print("numbers is not empty")

But I don't typically encourage relying on truthiness for testing for non-zeroness though. I'd usually rather see n % 4 != 0 rather than n % 4.

However, I'm not sure how common this behavior is in other code and I don't know whether we should assume that the function returns True specifically rather than simply a "truthy" value. I'd tend to err on the side of accepting truthy values rather than exclusively True, even though I'd personally implement this function to return True/False specifically.

Here's a semi-related StackOverflow question with answers from Alex Martelli and Guido van Rossum. Unfortunately it doesn't answer this specific question directly (should this function return a truthy value or specifically True).

[jgomo3]

I think the key is in this question you do:

I don't know whether we should assume that the function returns True specifically rather than simply a "truthy" value

And it is easy: how do you imagine client code using the function is_leap_year:

if is_leap_year(1996):
    do_something()

or

if is_leap_year(1996) is True:
    do_something()

[behrtam]

You could always write if is_leap_year(1996): do_something() whether you return True or a truthy value.

Okay, let's try to look at this from another angle. How would a solution to leap year look like that returns a truthy value instead of True/False?

[jgomo3]

You could always write if is_leap_year(1996): do_something() whether you return True or a truthy value.

In that case, I would say then that the tests should accept solutions that return truthy values instead of True/False.

Okay, let's try to look at this from another angle. How would a solution to leap year look like that returns a truthy value instead of True/False?

This is an example. It returns for the year 1996 the value 96.

For 1997, 1998, 1900 and 2400 returns False, False, False, True respectively.

def is_leap_year(year):
    return not year%4 and year % 100 or not year % 400

This example would be accepted if the tests would check with assertTrue and assertFalse instead of assertIs.

I would add another point of view also.

• What does it means assertTrue?
• What does it means assertIs?
• And what are our intentions on the test: Do we want to know if the objects are the same (using assertIs) or we want to know if the result is True or False?.

There is a recommendation in the same documentation on assertTrue which promotes the use of other assert method thant assertTrue if it is more specific. It gives the following example: use assertEqual(a == b) instead of assertTrue(a == b). The reason is simply because of a better error messages on failures, but it is a reason.

Well, then, we could argue the same for assertIs. Use assertTrue(exp) instead of assertIs(exp, True) because is a more specific method.

As a curiosity, note how the implementation of assertTrue wraps the expression evaluated with bool(exp).

[behrtam]

>>> def is_leap_year(year):
...     return not year % 4 and year % 100 or not year % 400
...
>>> [is_leap_year(y) for y in [1996, 1997, 1998, 2004, 2400]]
[96, False, False, 4, True]
>>>

I also tried to come up with an truthy example and only found solutions like yours that only sometimes return truthy values. So is our intention to encourage ugly solutions that return Booleans and Integers?

The error messages only differ in one character which is either upper or lower case:

======================================================================
FAIL: test_leap_year (__main__.YearTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "foobar.py", line 8, in test_leap_year
    self.assertIs(is_leap_year(1996), True)
AssertionError: False is not True

======================================================================
FAIL: test_leap_year (__main__.YearTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "foobar.py", line 9, in test_leap_year
    self.assertTrue(is_leap_year(1996))
AssertionError: False is not true

Note: I just try to play devil's advocate to find out what the consequences are of this change, but maybe I'm overdoing it. All existing solutions would still be green and those that would newly be accepted would also be doing the right thing. So, maybe we should just merge it.

[rootulp]

I think this test:

self.assertTrue(is_leap_year(1996))

is clearer than this test:

self.assertIs(is_leap_year(1996), True)

Unfortunately, using assertTrue only tests for truthy values and I think this implementation:

def is_leap_year(year):
    return not year % 4 and year % 100 or not year % 400

is far less easy to reason about than an implementation that explicitly returns True or False. I think encouraging students to return True or False from is_leap_year() leads to higher code quality.

[behrtam]

I totally agree with @rootulp. Normally assertTrue() should be preferred as it's more explicit but in this special case assertIs() should lead to better code quality.