Assertion helper added

[Kacppian]

• Helper to replace assert added.

[jkimbo]

This looks great! Thanks @Kacppian

[syrusakbary]

First, thanks a lot for your work @Kacppian.

After reviewing this PR, I sincerely don't prefer this new syntax, since with the assert the code was much more legible/readable.
This was my fault since I embraced the change proposed by @danpalmer without questioning if the readability was going to be better or not (#784), maybe this PR is approached differently to what Dan suggested:

Instead of:

assert something is not None, "Something was unexpectedly None"

We should do this:

if something is not None:
    raise AssertionError("Something was unexpectedly None")

What we have in the PR:

raise_assertion_if_not(
   something is not None,
   "Something was unexpectedly None"
)

I would love to get more input into this, so comments are more than welcome!

[danpalmer]

Hey sorry for not looking at this sooner. I agree with Syrus, the helper feels unnecessary. It basically just abstracts out an if statement, which I think makes the code less readable.

[Kacppian]

Thank you for the comments. Should I fall back to not having the helper method?

[jkimbo]

In JavaScript the invariant function is quite commonly used (https://github.com/zertosh/invariant) and I've found that I makes my code more readable. Would changing the method name to be more concise help?

[danpalmer]

@Kacppian in my opinion, we should use the form:

if condition:
    raise AssertionError(message)

And I think we should do this at the call sites, not in a function.

@jkimbo I think that makes more sense in the JS ecosystem where everything is libraries and functions all the way down, but in Python it's much expected that you do the obvious thing rather than wrapping up potentially unexpected effects.

In this case I don't think there's any benefit introduced by wrapping up AssertionErrors - it really does just feel like turning an if statement into a function - that requires an extra import, possibly a test, documentation, knowledge transfer, and understanding by new developers. An if/raise is just standard usage of two core parts of the language.

[Kacppian]

I used the suggested construction in #794 and closed it later because you'd suggested I use a helper method. Never mind, I'll revert back to it.

[danpalmer]

@Kacppian apologies for the back and forth. I disagree with @dan98765's suggestion on that PR, and it appears that @syrusakbary does too. I'm afraid we're not a cohesive team, just people who happen to have an interest in this library!

[Kacppian]

I'm extremely sorry. I didn't mean it to come out like that. Just mentioning the reason I've done it this way.

[danpalmer]

No problem! I didn’t read it as rude, and it is a valid criticism.

…

On 29 Aug 2018, at 18:39, Kaushik Asp ***@***.***> wrote: I'm extremely sorry. I didn't mean it to come out like that. Just mentioning the reason I've done it this way. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[Kacppian]

Cool then, I'll remove the helper method and make the necessary changes.

[jkimbo]

I agree that the unwrapped form is the one to go for because it's unambiguous and requires no extra
domain knowledge. However I do think it hurts readability because it draws attention to what are just sanity checks. IMO the best form is the assert one, it's just a shame that python doesn't respect it in production environments!

[morenoh149]

needs rebase

[morenoh149]

needs love ;)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.