Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Branching on all cases of an enum should be Mypy-enforced #49

Closed
ysangkok opened this issue Oct 13, 2020 · 2 comments
Closed

Branching on all cases of an enum should be Mypy-enforced #49

ysangkok opened this issue Oct 13, 2020 · 2 comments

Comments

@ysangkok
Copy link

It is possible to have Mypy complain if a case is missed when branching on an instance of an Enum, see mypy issue 6366.

This could be used in psbt.py , function check_witness_and_nonwitness_utxo_in_sync where all cases of PSBT_InKeyType are matched. Instead of raising AssertionError, _assert_never could be called, which would trigger a Mypy error on compile time. If Mypy isn't run, it still fails on runtime. So there is no disadvantage except the reader needing to learn the NoReturn type.

It could also be used in PSBT_Input.merge, but it seems that Mypy is not smart enough to detect that "(a and b) or (not a and b), (a and not b), (not a and not b)" is always true. It would probably work to introduce an enum with 4 cases for this or maybe it would work with a (bool, bool) tuple, but I don't know if Mypy is smart enough for this, and I don't know if you think it is worth it.

@dgpv
Copy link

dgpv commented Oct 13, 2020

Yes, using this for enums is a good idea.

For PSBT_Input.merge I don't think that introducing enum would add value. The detection of unhandled enum case is beneficial because of the separation of the data type (the enum) and the code that handles it. The problem arises when the enum was extended, but handling code did not change.

In PSBT_Input.merge case the data type is implicit in the operation, because it emerges out of combination of two inputs. Because this implicit data type is not separated with the code, the handling of it is always local to this context, and changing the data type implies changing the code. I think that formalizing data type there would unnecessary add indirection (and hence, complexity).

@dgpv
Copy link

dgpv commented Oct 17, 2020

The change is in #51 and is scheduled to be included in v1.1.2, closing this issue.

@dgpv dgpv closed this as completed Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants