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

Talking errors in casting failures #903

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

davidepaolotua
Copy link
Contributor

Implements #893. There is a breaking change, though. Previously, calling FailedCast#value would always return a nil value, without providing any clue on which value was actually breaking.
Now, it raises a FailCastError with a talking error message, in the form of - for instance - "Value ->Invalid<- is not a valid constant for enum WhateverEnum".

Btw, is there an "easy" way to test avram modifications in a dummy lucky app?

Implements luckyframework#893. There is a breaking change, though.
Previously, calling FailedCast#value would always return a nil value, without providing any clue on which value
was actually breaking.
Now, it raises a FailCastError with a talking error message, in the form of - for instance - "Value ->Invalid<- is
not a valid constant for enum WhateverEnum".
@jwoertink
Copy link
Member

Hmm... I'm not sure about this one 🤔 The FailedCast was never intended to be an error. It's meant to just be a type check that it didn't know how to convert. By setting these values to nil, then your operation can handle the errors as you need. With this raising now, then you would never get to your operation in a full Lucky app. It would fail out before you had a chance to gather up all of the validation errors. And since exceptions are meant to be handled in your Lucky app's Error::Show action, this would end up breaking some of that convention.

I do see why you went this route though. I think I want to sit on this a little longer and see if there's an alternate way we can fix the issue without making some breaking changes here. Thanks for getting this started!

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

Successfully merging this pull request may close these issues.

2 participants