Proposal for improved safety of assert_that #149
Conversation
| # Warn if we got a Matcher first | ||
| if isinstance(actual, Matcher): | ||
| warnings.warn("arg1 should be boolean, but was {}".format(type(actual))) | ||
| # Or if actual and matcher are both truthy strings |
There was a problem hiding this comment.
You aren't checking if matcher is truthy.
| warnings.warn("arg1 should be boolean, but was {}".format(type(actual))) | ||
| # Or if actual and matcher are both truthy strings | ||
| if isinstance(actual, str) and isinstance(matcher, str) and actual: | ||
| warnings.warn("assert_that(<str>, <str>) only evaluates whether the " |
There was a problem hiding this comment.
Perhaps we could suggest resolving the ambiguity by passing the reason as a named parameter, in the event that they truly intended to do a truthiness assertion? assert_that(<str>, reason=<str>)
| if isinstance(actual, str) and isinstance(matcher, str) and actual: | ||
| warnings.warn("assert_that(<str>, <str>) only evaluates whether the " | ||
| "first argument is True, so the second string is always " | ||
| "ignored. Make sure to use an explicit matcher for the " |
There was a problem hiding this comment.
It isn't always ignored. It is used as the reason string in the event that the first string is falsey.
| # It really only makes sense for matcher to be a Matcher instance or a | ||
| # string (actually the 'reason'). Anything besides that is more likely to | ||
| # be a mistake than anything else, so properly warn. | ||
| if not isinstance(matcher, str): |
There was a problem hiding this comment.
We shouldn't warn here if matcher is _unassigned, since it means they are just asserting the truthiness of actual.
| @@ -25,7 +26,7 @@ def assert_that(assertion: bool, reason: str = "") -> None: | |||
| ... | |||
|
|
|||
|
|
|||
| def assert_that(actual, matcher=None, reason=""): | |||
| def assert_that(actual, matcher=_unassigned, reason=""): | |||
There was a problem hiding this comment.
Can this perhaps be simplified? (Forgive my somewhat pseudocode.)
# second parameter was a Matcher
if isinstance(matcher, Matcher):
return _assert_match(actual, matcher, reason)
# first parameter was a Matcher, the caller maybe passed them in the wrong order? (existing warning)
if isinstance(actual, Matcher):
warnings.warn('...')
# second parameter was not provided, third parameter may have been provided
if matcher is _unassigned:
return _assert_bool(actual, reason)
if reason:
warnings.warn('reason string will be ignored...')
# If second parameter was not a string, the caller likely thought they were doing an equality check.
# If both parameters are strings, it is ambiguous whether the caller thought they were doing an equality check,
# or a truthiness check with a reason string. Err on the side of caution and log a warning.
# If a truthiness check was desired, they can resolve the warning by passing the reason as a named parameter.
if not isinstance(matcher, str) or isinstance(actual, str):
warnings.warn('possible misuse...')
return _assert_bool(actual, matcher)I think we should avoid introducing a TypeError at this moment, because that is a breaking change. It would be nicer to have an intermediate release that warns about suspicious usages, allowing people to fix these issues without having to fix everything at once.
|
The CI setup has been significantly revamped since this was created (and I have not had time to think through it in the interim). Please rebase this PR and let's see where it shakes out. I'm cautiously okay with the basic idea of warning in the limited case where there is a single-arg call to |
@offbyone I don't understand what you mean by this. Single arg calls are rare (in my experience) and aren't the problem. The root problem is that our developers (and reviewers) continue to erroneously expect |
Alternative for #148 and #147 that more specifically targets usage patterns that are highly likely to be mistakes.