Remove unreachable code

[davesque]

Also, remove unnecessary else clause that tends to obscure that problem.

What was wrong?

There was an assert False statement which was unreachable. It was clearly expected that this statement was not supposed to be reached in a "normal" situation. However, I don't think it was realized that the statement could never be reach in any situation.

How was it fixed?

Deleted it.

Cute Animal Picture

[carver]

Yeah, I feel ambivalent. There are times when I think that having explicit branching and an explicit "unreachable" statement are more robust to future breakage.

In such a short, straightforward method, I see the elegance in the approach with less branching. So then the only thing that bothers me is that it's a pattern elsewhere.

So in my ambivalence, I leave it up to you :) 👍

[davesque]

@carver So is the unreachable assert meant to hint that the function should terminate in one of the if branches? If so, that makes more sense.

However, I still do kinda feel like that pattern is confusing. It seems like you'd need to explain it to anyone who encounters it, whereas returning false in the except block or true in the normal function body is clear enough at a glance.

Although it does kinda bother me if the pattern is used elsewhere except here...hmm.

[carver]

@carver So is the unreachable assert meant to hint that the function should terminate in one of the if branches? If so, that makes more sense.

Exactly, that's how I read the unreachable assert.

However, I still do kinda feel like that pattern is confusing. It seems like you'd need to explain it to anyone who encounters it, whereas returning false in the except block or true in the normal function body is clear enough at a glance.

Yeah, this is why I don't feel strongly about it.

Although it does kinda bother me if the pattern is used elsewhere except here...hmm.

It's not used a whole lot, but has become a bit of a pattern. I searched for "unreachable" (which didn't even find this usage, of course), and only found two. I think the explicit message is much better than the blank assert. For example:

web3.py/tests/core/eth-module/test_accounts.py

Lines 81 to 85 in 3d14361

	if request.param == 'instance':
	return web3.eth.account
	elif request.param == 'class':
	return Account
	raise Exception('Unreachable!')

and even better:

web3.py/web3/_utils/transactions.py

Lines 126 to 127 in 3d14361

	else:
	raise Exception("Unreachable path: transaction's 'data' is either set or not set")

[davesque]

Mkay, concerns noted. Since this is only a few lines of code, I'll go ahead and merge this.