search

beberlei / assert

Thin assertion library for use in libraries and business-model
Other
2.41k stars 188 forks source link

BC break in 2.7.4: stringify() method suddenly returns different result for booleans #226

Closed bicpi closed 7 years ago

bicpi commented 7 years ago

Changing the stringify() method in https://github.com/beberlei/assert/commit/70c865382bddeff1686738ba2c86fae2e50cbc30#diff-4066b84b483f6098378f612ecc64f381 introduced a BC break that broke my unit tests.

Stringifying a boolean results in 1 instead of <TRUE> as before because the early returns were removed. <TRUE> is correctly assigned as a result first, but then overwritten by the is_scalar() check because a boolean is a scalar, too.

I could do a PR to revert if you like, but what was the intention of this change?

rquadling commented 7 years ago

Just waiting on Travis to confirm all is well before a release.

Thank you for reporting this.

rquadling commented 7 years ago

https://github.com/beberlei/assert/releases/tag/v2.7.6 released.

bicpi commented 7 years ago

@rquadling Thanks for fixing this so fast 👍

bicpi commented 7 years ago

Btw, what was bad with the early returns, isn't this a best-practice? More readable, less brittle, faster.

rquadling commented 7 years ago

Normally early exits are for error states.

A single return point makes more sense to me as the result is not an error.

bicpi commented 7 years ago

Error states? I think returns in object methods are there to return ... the return value :-) It's not the return value of a CLI command.

The earlier one can return the better, it avoids unnecessary nesting, hard to read else conditions etc.

Checkout http://williamdurand.fr/2013/06/03/object-calisthenics, nice read.