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

Field error refactor #80

Merged
merged 3 commits into from
Sep 2, 2017
Merged

Field error refactor #80

merged 3 commits into from
Sep 2, 2017

Conversation

mhallin
Copy link
Member

@mhallin mhallin commented Aug 8, 2017

An initial attempt at solving #69 and #40. It pretty much matches what I wrote here: #40 (comment).

To ease construction of custom error objects, I built a small macro to construct Value instances from JSON syntax. This might be a bit out of place, in which case it can be removed from the PR. I also don't think it's going to be used for other purposes, but whatever :)

This obviously breaks all users of .to_field_err because of the renaming and places where you return Err("string literal".to_owned()) from a field, but all instances of e.g. let txn = executor.context().db.transaction()? should continue to work.

@rushmorem
Copy link
Contributor

@mhallin Is there any particular reason you are not merging this in?

@theduke
Copy link
Member

theduke commented Aug 14, 2017

Probably waiting for feedback.

I'll try to carve out some time to go over all MRs tomorrow.

@mhallin
Copy link
Member Author

mhallin commented Aug 15, 2017

Yeah, I personally don't have a use case for sending extra data with errors so I wanted some input from someone else if this looks reasonable and solves their problem.

@mhallin mhallin merged commit b3ea59c into master Sep 2, 2017
@mhallin mhallin deleted the field-error-refactor branch September 2, 2017 08:44
@theduke
Copy link
Member

theduke commented Sep 5, 2017

@mhallin I finally looked over the PR and it looks good to me. Should cover the use case of detailed errors well.

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