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

results: Add Opt/Result converters #177

Merged
merged 4 commits into from
May 11, 2023
Merged

results: Add Opt/Result converters #177

merged 4 commits into from
May 11, 2023

Conversation

arnetheduck
Copy link
Member

Add optError, optValue, okOr and errOr to convert back and forth between Opt and Result - these conversions end up being more common than others since "trivial" success/fail-style API often use Opt while combining such API into combined operations tends to prefer richer error reporting.

@arnetheduck
Copy link
Member Author

Notably, the template orErr already covers the Opt[T] -> Result[T, E] case and perhaps more broadly.

The "missing piece" here is converting an option to a a result, placing the value of the option in the error of the result, ie flipping them.

Copy link
Contributor

@zah zah left a comment

Choose a reason for hiding this comment

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

The names okOr and errOr seem quite ambiguous to me and errOr is a no-go, considering that Nim can be style insensitive.

okOr could simply be named asResult().

errOr is quite a strange API and thus, it's quite difficult to come up with a suitable name. When would I want to optionally create an error value our of the "successful" result of another operation? Perhaps only in some overly monadic code such as:

  return someApiResturingResult().optError.errOr:
     # a large block of code computing the successful result

... but I could have easily written this in a more natural and performant way.

stew/results.nim Outdated Show resolved Hide resolved
stew/results.nim Outdated Show resolved Hide resolved
@arnetheduck
Copy link
Member Author

arnetheduck commented Apr 27, 2023

The names okOr and errOr seem quite ambiguous to me and errOr is a no-go, considering that Nim can be style insensitive.

Yeah, I'm not too happy about errOr either, but they logically fit the rest of the module - ok and err consistently construct a Result and these are constructors of Result that add either an error or a value to an Opt.

okOr could simply be named asResult().

asResult is not great for two reasons: it does not follow the ok/err convention for construction and it gives primacy to the "value" side. ok highlights the constructive nature of the call and the Or part is used in similar way as in valueOr, ie to provide a resolution when there is no value available. asResult(option, error) reads a bit like it would construct Result[Opt[T], E] rather than Result[T, E] - the Or is needed there to "unpeel" the Opt as part of the process of "adding" Result.

errOr is quite a strange API and thus, it's quite difficult to come up with a suitable name. When would I want to optionally create an error value?

The main case I can think of is after you've used optErr and want to go back to Result.
Granted, this is not the most common of operation and "balance" is probably the best reason I could cite. It's not a strong case and leaving it for another day might be just the thing.

@zah
Copy link
Contributor

zah commented Apr 27, 2023

asResult reads as a conversion, not construction, so I'm not convinced that Result[Opt[T], E] is the expected return type. It's actually the ok family of functions that are constructors in the sense that they lift the input value to a monadic type (the new okOr in an outlier in this sense).

Other possible names are withErr or replaceErr which can be defined for all Results.

@arnetheduck
Copy link
Member Author

The Result-general replaceErr already exists (named mapErr) and can also be used for this purpose (ie myOpt.mapErr(proc (): E = E()) is equivalent to okOr - the Or variations generally imply use a lazily evaluated template in the whole Result module as is the case here.

outlier

In okOr, ok is in balance thanks to Or: Or removes a layer of Result/Opt (as in valueOr, ok adds it back - why is it outlier in this case?

In terms of As naming, were we not thinking about reserving that for plain conversions? ie myInt as string which imply equivalence in value but not in type? this is not the case for asResult which adds stuff (the error).

@zah
Copy link
Contributor

zah commented Apr 27, 2023

I don't feel I'm spending my time productively continuing this conversation, so feel free to ignore my feedback if you are decided on okOr.

But I don't see how the Or suffix implies that a layer is removed. If valueOr is a variation of value, they both return the same layer - a T extracted out of the Result[T], so Or is not a suffix that navigates the layers.

Otherwise, I was aware of mapErr, but replaceErr is a potential shortcut, just like you have other shortcuts, such as mapCast, which are significantly less generally useful.

The plain as is a direct conversion indeed, but I think converting Opt to a Result is very close to such a direct conversion - you are merely changing the error annotation which is usually relevant only for the end user of the software - for the rest of the code the new Result value has very similar semantics.

@arnetheduck
Copy link
Member Author

If valueOr is a variation of value,

Oh, now I see what you meant, let me think.

The plain as is a direct conversion indeed

fwiw, I've considered valueAsOpt and errorAsOpt instead of optValue / optGet - these would be direct enough.

Add `optError`, `optValue`, `okOr` and `errOr` to convert back and forth
between Opt and Result - these conversions end up being more common than
others since "trivial" success/fail-style API often use Opt while
combining such API into combined operations tends to prefer richer
error reporting.
`orErr` already covers `okOr`, `errOr` is controversially named _and_
not really that useful
@arnetheduck
Copy link
Member Author

shortcut

This situation resolved itself rather nicely in that the relevant converter already existed with a good name: myOpt.orErr("error") already gives the expected Result[T, string]. I removed the controversial errOr - it could be named errValueOr maybe, but that's a stretch and its utility is limited nevertheless.

what remains to consider there is the other direction: optValue and optError - decent enough names, but maybe there's something better?

@zah zah merged commit 266e900 into master May 11, 2023
@zah zah deleted the opt-conv branch May 11, 2023 13:34
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