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

Added Error::Term for returning {:error, <term>} #252

Merged
merged 1 commit into from
Nov 11, 2019

Conversation

elbow-jason
Copy link
Contributor

Hi! I added a variant to return any encoder as an error. I think could help ergonomics and help return more meaningful errors from our native Rustler interfaces to Elixir.

Feedback is appreciated. :)

@filmor
Copy link
Member

filmor commented Oct 16, 2019

Where would you return something arbitrary (i.e. not {error, _}) to indicate an error?

@elbow-jason
Copy link
Contributor Author

That's a great question. Idiomatically, I would only ever return {:error, _} as an error.

Perhaps this function would be better as something like ReturnErrorTuple(Box<Encoder>) and return {:error, _}.

It would be pretty strange to return:

ReturnTerm(Box::new((atoms::ok(), "some ok result wrapped in an error")))

@filmor is your concern something like the Rust code above?

@filmor
Copy link
Member

filmor commented Oct 17, 2019

In line with the existing options, I think you should just add a Error::Term(X) that results in a returned error {error, X}.

@elbow-jason
Copy link
Contributor Author

@filmor Agreed; {:error, term} is preferrable. I should have time to get this done on Friday. Thanks for the feedback!

@scrogson
Copy link
Member

@elbow-jason...do you have time to finish this? If not, let me know and I can take it to the finish-line.

@elbow-jason elbow-jason force-pushed the add-return-term-to-error branch 2 times, most recently from f2f6be3 to c36d699 Compare October 31, 2019 04:06
@elbow-jason elbow-jason changed the title Added ReturnTerm to Error for returning arbitrary terms as errors Added Error::Term for returning {:error, <term>} Oct 31, 2019
@elbow-jason elbow-jason force-pushed the add-return-term-to-error branch from c36d699 to 5eb1645 Compare October 31, 2019 04:14
@scrogson scrogson self-requested a review October 31, 2019 14:53
Copy link
Member

@scrogson scrogson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with these changes 👍

I think we should wait to merge this after #264 is merged. There will only be a small number of conflicts to fix in this PR vs the other way round.

@filmor
Copy link
Member

filmor commented Oct 31, 2019

Hmm, I just noticed that Error::Atom("some_str") actually returns some_str and this made me wonder whether it wouldn't be good to make this one also stricter by making it return {error, some_str}. The only halfway valid case to return a bare atom is forwarding of a badmatch or badarg and a totally invalid case is returning Error::Atom("ok"). Shouldn't be part of this PR, though.

@elbow-jason
Copy link
Contributor Author

@filmor Error::Term covers the use case you described for Error::Atom. I think what you are recommending is the deprecation of Error::Atom.

@scrogson
Copy link
Member

Given that Error::Term(term) returns wrapped values, I think Error::Atom("error") is still relevant though.

@filmor
Copy link
Member

filmor commented Nov 1, 2019

@elbow-jason No, I'm recommending a change of semantics for Error::Atom as I don't like it when libraries return a bare atom instead of {error, Description} as an error, but like @scrogson said, it's quite useful to have the possibility to return something like this without boxing :)

@scrogson
Copy link
Member

scrogson commented Nov 9, 2019

@elbow-jason now that #264 has been merged, feel free to fix up this PR so we can merge 👍

@elbow-jason elbow-jason force-pushed the add-return-term-to-error branch from 5eb1645 to f327a19 Compare November 11, 2019 16:37
@scrogson scrogson merged commit 9353f7f into rusterlium:master Nov 11, 2019
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.

3 participants