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

Values::InvalidEnvelopeVal#definition missing? #212

Open
natec425 opened this issue Feb 1, 2020 · 3 comments
Open

Values::InvalidEnvelopeVal#definition missing? #212

natec425 opened this issue Feb 1, 2020 · 3 comments

Comments

@natec425
Copy link
Contributor

natec425 commented Feb 1, 2020

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!

@kputnam
Copy link
Owner

kputnam commented Feb 1, 2020

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.

@natec425
Copy link
Contributor Author

natec425 commented Feb 1, 2020

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

@kputnam
Copy link
Owner

kputnam commented Feb 2, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants