is_not + greater_than passes for non-comparable types

[rittneje]

The fix for #185 made it so that OrderingComparison returns False rather than throwing a TypeError when the two types in question are not comparable (e.g., str and int). However, this new behavior is rather unexpected, as it causes is_not to return True. For example:

assert_that("a", is_not(greater_than(1)))

In reality, PyHamcrest needs to incorporate ternary logic in order for such comparisons to be treated in a consistent manner.

In the meantime, I suggest that #199 be reverted.

[brunns]

I'm not sure I agree. It is true that "a" is not greater than 1 - it's not comparable to it.

And introducing ternary logic feels like it would be a huge can of worms. is_not() is a simple, easy to explain matcher, and I think I prefer it that way.

[rittneje]

There are two expectations this violates.

1. is_not(greater_than(1)) is logically equivalent to less_than_or_equal_to(1)
2. assert_that("a", is_not(greater_than(1))) is logically equivalent to assert_that(not("a" > 1))

In addition, by using greater_than(1), there is the implication that the value in question is supposed to be an int. So this change will lead to bugs being undetected. To that end, it is always preferable for PyHamcrest to choose the interpretation that leads to false negatives (i.e., tests failing that shouldn't) over false positives (i.e., test passing that shouldn't).

Another way to resolve your original problem without introducing ternary logic is to be more explicit about what you actually meant.

assert_that(['a',4], contains_inanyorder(all_of(instance_of(int), greater_than(0)), 'a'))

[offbyone]

is_not(greater_than(1)) is logically equivalent to less_than_or_equal_to(1) is an assumption that is conditional on the value being compared being comparable at all. Hamcrest does not, in general, raise exceptions when a matcher is applied to the wrong type; in those cases, it doesn't match... which is fine.

assert_that("a", is_not(greater_than(1))) is logically equivalent to assert_that(not("a" > 1)) is not the same thing either; in one case you are evaluating the comparator yourself, with everything that implies, and in the other you're making the broader statement regardless of the types' effects.

Trying to handle a ternary case in an inverting binary matcher is something I don't believe we should do in Hamcrest; I think the consequences in terms of complexity will be far worse.

[rittneje]

@offbyone The two statements I listed above are perfectly reasonable expectations. You are choosing the worse interpretation that will lead to bugs being silently ignored. To me, that is not an acceptable stance for a testing framework.

[offbyone]

You are making a small, but critical, category error: Hamcrest is a matching framework, often useful in testing.

The two statements you make are reasonable expectations when applied to numerical operators, or when both entities being compared are numerical, but those assumptions don't hold when the types are incompatible.

[offbyone]

I'll take your thumbs up to mean you disagree, but this is not a bug in hamcrest as it is intended, and I'm going to close this issue.

[rittneje]

@offbyone Not just disagree, I will now be actively campaigning to remove this library from our codebases. It is clear that between this and #147, PyHamcrest is not fit for use.

[offbyone]

Darn. Okay!