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

Add comparison/migration guide to failure #50

Closed
shepmaster opened this issue Apr 18, 2019 · 3 comments · Fixed by #65
Closed

Add comparison/migration guide to failure #50

shepmaster opened this issue Apr 18, 2019 · 3 comments · Fixed by #65
Labels
maintenance Keeping the wheels turning

Comments

@shepmaster
Copy link
Owner

This will be obviously biased, but we can compare against what is in the documentation as best we can.

@shepmaster shepmaster added help wanted Extra attention is needed maintenance Keeping the wheels turning feedback requested User feedback desired on a design decision labels Apr 18, 2019
@lnicola
Copy link

lnicola commented Apr 25, 2019

I ported some code from failure. Some remarks:

  • I was using tuple enum items, and I had to name the fields because the documentation didn't say whether they were supported or not
  • It's not obvious that the source field is mandatory (fail is nifty, though)
  • I'm somewhat worried about the size of the Results. IIRC the compiler is bad at optimizing them, and sometimes it might be better to do an allocation instead.
  • I didn't look too much into it, but I'm not sure what eager_context is about

@shepmaster
Copy link
Owner Author

shepmaster commented Apr 25, 2019

the documentation didn't say whether they were supported or not

They are not, but I’m open to discussion about it. I've opened #61 to talk about it.

I should also add this fact explicitly to the docs. You do get a specific error if you try to use tuple variants though.

the source field is mandatory

It is not; but if there’s no source field you can’t use context (where would the Result::Err go?). Instead you just use ContextSelector { fields }.fail(). Can you expand a bit more on wwhat you mean by "mandatory"?

  • the size of the Results

Do you mean Result or the enum Error type? I would expect that the size of the enum is max(size-of(variants)) + 1 byte + small amount padding) if you have less than 256 variants. There is some discussion elsewhere about better supporting boxing source errors which might help a bit.

  • what eager_context is about

TL;DR, context is used with ? and eager_context is used without ?.

@lnicola
Copy link

lnicola commented Apr 26, 2019

It is not; but if there’s no source field you can’t use context (where would the Result::Err go?). Instead you just use ContextSelector { fields }.fail(). Can you expand a bit more on wwhat you mean by "mandatory"?

I don't have anything more to add to my comment in #10 (comment). Again, I don't mind the way it works, it's just an issue I ran into because I wasn't aware of fail().

Do you mean Result or the enum Error type?

The Error enum, as the variants could presumably grow pretty large, depending on how many errors you're nesting.

shepmaster added a commit that referenced this issue May 5, 2019
@shepmaster shepmaster removed help wanted Extra attention is needed feedback requested User feedback desired on a design decision labels May 5, 2019
shepmaster added a commit that referenced this issue May 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Keeping the wheels turning
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants