stevearc / flywheel

Object mapper for Amazon's DynamoDB
MIT License
128 stars 25 forks source link

Minor: developer usability, `Query.one()` and `ValueError` #55

Closed danfairs closed 7 years ago

danfairs commented 7 years ago

The docstring for Query.one() says the following:

        Return the result of the query. If there is not exactly one result,
        raise a ValueError

This is technically true - it actually raises one of two subclasses of ValueError, DuplicateEntityException or EntityNotFoundException. The trouble is, the docstring hints that the caller should catch ValueError directly - and the implementation of .one() calls .all(), which can wind up calling Condition.query_kwargs(). This method can end up raising a ValueError directly, if the query has bad arguments. This then accidentally gets caught at the top level by the caller of Query.one() - which was catching ValueError to detect a the query not returning any results.

Do you think it's worth clarifying the docstring (and any other docs) to explicitly call out the DuplicateEntityException and EntityNotFoundException classes, and encourage those to be caught directly, rather than catching ValueError?

stevearc commented 7 years ago

Oh, sure yeah it certainly does make it seem like you should maybe catch a ValueError when that is not the case. I'll clarify in the docstring. I don't think it appears anywhere else.