mikeshultz/ledger-eth-lib

feat request: support HexBytes more

Closed this issue · 5 comments

so many places have assert type(thing) == bytes
if we did assert isinstance(thing, bytes), it'd allow HexBytes

But also, assertion errors are not the best to encounter, the message is not helpful without digging into the tb; a TypeError("Expecting bytes, not blah blah") would be better for the lib

I'm pretty hesitant to jump on HexBytes. Would kind of prefer to remain using primitives and not add a dependency. Keep this lib as simple as possible and let the consumer make decisions on things like that.

The assertion errors are all (I think) just to shut up mypy, not so much as input validation. I'm curious, where'd you run into it? It might be a bug.

isinstance() will work for mypy too! And you dont have to explicitly support HexByes, it will just work.
basically all mypy assertions that use type() instead of instance

isinstance() will work for mypy too!

I get that, I only brought that up to say that you shouldn't be seeing these assertion errors. They aren't for users.

And you dont have to explicitly support HexByes, it will just work.

What does this mean?

The assertions will be there unless you compile in optimized mode.
And I mean I will be able to pass sub-classes instead of the asserted type and everything will work but you won't have to include the mentioning of HexBytes anywhere because itll still work but it wont fail the type check

Oh, I think I misunderstood. isinstance(x, bytes) would allow HexBytes. You were not requesting adding isinstance(x, HexBytes) to allow derivatives of HexBytes. Yeah, I'm good with that.

Also, deja vu.