stupidedi icon indicating copy to clipboard operation
stupidedi copied to clipboard

Values::InvalidEnvelopeVal#definition missing?

Open natec425 opened this issue 4 years ago • 3 comments

Hi hi,

Is the definition method missing from the Values::InvalidEnvelopeVal? It seems like AbstractVal assumes all derived classes implement definition.

Please let me know if I'm misunderstanding something. If I'm correct that it is missing, I'd be happy to take a swing at a PR.

Thanks for your time!

natec425 avatar Feb 01 '20 20:02 natec425

Yes I think there are a few cases where a method isn't implemented in a "concrete" subclass. It's usually just because the method doesn't have a sensible implementation. Ideally the types would've been designed to avoid this from happening, but I've only been able to keep it to a minimum.

How did you run into it? I would want to see what's going on at the call site if stupidedi itself is calling #definition on an InvalidElementVal. The error might just be that it was called in the first place, like the caller incorrectly assumes everything has a definition. It probably makes sense to just return nil, but hard to say without knowing more.

kputnam avatar Feb 01 '20 20:02 kputnam

This didn't actually come up at run time. It was found by a type checker 😁

natec425 avatar Feb 01 '20 21:02 natec425

I would accept a PR that makes it throw NoMethodError with a message that says InvalidEnvelopeVal doesn't have a definition.

That's assuming you can change InvalidSegmentVal to do the same without any breakage (now it returns nil). If that introduces test failures then I'd be fine with both of them returning nil.

kputnam avatar Feb 02 '20 21:02 kputnam