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

Rename ResultExt::to_field_err? #69

Closed
theduke opened this issue Jul 29, 2017 · 4 comments
Closed

Rename ResultExt::to_field_err? #69

theduke opened this issue Jul 29, 2017 · 4 comments

Comments

@theduke
Copy link
Member

theduke commented Jul 29, 2017

The method name ResultExt::fo_field_err is a bit confusing, since it implies an Err return type.

We could leave it as is, or rename it to to_field_result or to_field_res.

If we rename, it should definitely be before 1.0.

Thoughts, @mhallin ?

@mhallin
Copy link
Member

mhallin commented Jul 29, 2017

Sounds reasonable, but while we're on the topic of field errors... I feel that error handling is a bit unergonomic right now. For example, if you're doing stuff that can fail, you've got two options in practice:

  • Import juniper::ResultExt, write e.g. let txn = db.transaction().to_field_result()?;
  • Use jtry!, e.g. let txn = jtry(db.transaction());

Converting error types yourself feels like a shortcoming on Juniper's side; why should a user have to care about that? On the other hand, using the macro looks a bit weird when you're chaining operations that can fail: jtry!(jtry!(File.open(...)).read(...)).

So, I wonder if we could make the macros support any kind of Result<T, E> (or at least E: Display) and call .to_field_result() on the result automatically? Then we could maybe even remove the ResultExt trait and jtry! macro.

@srijs
Copy link
Contributor

srijs commented Aug 7, 2017

IIRC both the try macro as well as the ? operator provide type upcasting: https://github.com/rust-lang/rfcs/blob/master/text/0243-trait-based-exception-handling.md#exception-type-upcasting.

So an alternative could be to define a concrete type FieldError implementing From:

struct FieldError {
  message: String
}

type FieldResult<T> = Result<T, FieldError>;

impl<E> From<E> for FieldError where E: Display {
  fn from(err: E) -> FieldError {
    FieldError { message: format!("{}", err) }
  }
}

This would enable people to use the ? operator without to_field_result in field bodies:

field foo()  {
  let txn = db.transaction()?;
  ...
}

... as well as enable the use of try! instead of jtry!, meaning that both jtry and ResultExt could be deprecated.

@theduke
Copy link
Member Author

theduke commented Aug 7, 2017

Deprecating the macros is something we already talked about.

I'll write more here later, but just for now: #40 is also relevant for this.

This was referenced Aug 8, 2017
@theduke
Copy link
Member Author

theduke commented Dec 3, 2017

Both ResultExt and the jtry macro were removed in 0.9.

@theduke theduke closed this as completed Dec 3, 2017
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

No branches or pull requests

3 participants