verify() 's API is ambiguous

[ewjoachim]

This stems from pypi/warehouse#8562 and the downtime experienced in PyPI due to my own misunderstanding of the verify API.

So here's the thing: there are 2 pythonic ways to answer the question "was there a problem?":

• Either True(no problem) or False (a problem). This one is simple and lacks expressivity.
• Either pass or raise. This one is more precise because we get to tell exactly how this failed (cf Verifier.verify raises very general exceptions #51 ). As the zen of python says, "errors should never pass silently".

verify's API (as far as I can tell) is: True or raise.
This means:

• checking for the Truthy values returned by verify is useless and will most likely lead to dead code (it will never return False)
• checking for Truthy values without wrapping the code in a try/except block will result in crashes

Looking at the code:

pymacaroons/pymacaroons/verifier.py

Line 80 in abc36a6

return True

There is a single return value in the method and it returns the constant value True, so nothing else can ever be returned from this function, meaning the True bears no information, it could well have been a None, or no return value.

I think the best solution could be to stop advocating that there is a return value for the verify method, and document the use of wrapping it in a try/except(#51 will also be key to doing this). This way the code doesn't even have to change and it can continue returning True for backwards compatibility for as long as you wish. If you ever want to remove it, you can even return an object whose __bool__ method raises a DeprecationWarning and returns True (and also ideally override __is__ in case people wrote if verify() is True.
As of today, the README example doesn't wrap the verify() call in a try/except so it can be misleading. Also, it seems the functionnal tests that are advertised as documentation examples do only cover cases where this returns True.

[merwok]

(there is no __is__ method)

[ewjoachim]

Damn, I keep forgetting about that. And Bool isn't subclassable, so I guess there would be no way to raise on verify() is True but not on verify(). Well, too bad, let's return True for all eternity :) Or arbitrarily stop one way and heavily document it in the changelog and hope for the best. Anyway, the important part is to promise that we'll always raise if verification fails.

That being said, #51 will be quite an API break too, so for that matter, not returning True anymore could be done at the same time.

[woodruffw]

Warehouse has a defensive workaround for this, in pypi/warehouse#11885.

I have no strong preference for a return or raise-style API, as long as it's consistent 🙂, but to prolong the bikeshedding a little more: I'm personally a fan of Allow/Deny-typed APIs.

In other words, a hypothetical breaking API could look something like this:

Status = Union[Success, Failure]

def verify(macaroon, key) -> Status:
    ...

...where Success is a unit type with a truthy __bool__ (or optionally carry a success message), and Failure is a falsy type with a mandatory message (wrapping the inner verification or other exceptions, as necessary).