Skip to content

Conversation

arnetheduck
Copy link
Owner

@arnetheduck arnetheduck commented May 20, 2018

Comments welcome!

result.nim Outdated
ResultError = object of Exception

# TODO check that r is a Result
proc ok*(r: typedesc, v: auto): r =
Copy link

Choose a reason for hiding this comment

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

In theory, a more precise signature that should work is the following:

proc ok*(R: typedesc[Result], v: R.T): R

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah yes, that's nicer - the auto at the end is a habit from C++ to enable in-place implicit construction/conversion, for whenever that matters.. less common in Nim I guess

Copy link

Choose a reason for hiding this comment

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

I suspected this could be the motivation, but a converter will kick in with the precise signature as well. The difference is that the conversion will happen at the call-site of ok instead of instantiating ok with multiple types and applying a converter at the value: v assignment.

Copy link
Owner Author

@arnetheduck arnetheduck May 22, 2018

Choose a reason for hiding this comment

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

indeed, and in C++ that's pretty common and usually better (with perfect forwarding).. in nim, less of a difference I guess, but consider a cstring -> string - compared with T this form will save an operation, at the expense of more generated code.. no big difference functionally.. I guess including T might give a better error message maybe..

result.nim Outdated
## Initialize a result with a success and value
## Example: `Result[int].ok(42)`
r(isOk: true, value: v)
proc ok*[T, E](self: var Result[T, E], v: auto) =
Copy link

Choose a reason for hiding this comment

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

Perhaps you are using auto on purpose though?

Again, a more precise signature is

proc ok*[T, E](self: var Result[T, E], v: T)

or

proc ok*(self: var Result, v: self.T)

result.nim Outdated
## Example: `result.ok(42)`
self = Result[T, E].ok(v)

proc err*(r: typedesc, e: auto): r =
Copy link

Choose a reason for hiding this comment

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

Similar to ok (regarding the signature)

result.nim Outdated
## Example: `Result[int].err("uh-oh")`
r(isOk: false, error: e)

proc err*[T, E](self: var Result[T, E], v: auto) =
Copy link

Choose a reason for hiding this comment

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

Similar to ok (regarding the signature)

result.nim Outdated
## Example: `result.err("uh-oh")`
self = Result[T, E].err(v)

template isErr*[T, E](self: Result[T, E]): bool = not self.isOk
Copy link

Choose a reason for hiding this comment

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

I know this style hasn't gained many followers in the Nim community yet, but I usually recommend to use implicit generics:

template isErr*(self: Result): bool = not self.isOk

I think the shorter code is more visually pleasing and it's more resilient to re-factoring (You can make changes to the generic parameters without touching all the procs in the module).

Copy link
Owner Author

Choose a reason for hiding this comment

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

ah yes, this is nicer

result.nim Outdated
if self.isOk: self
else: other

template `+`*[T, E](self, other: Result[T, E]): untyped =
Copy link

Choose a reason for hiding this comment

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

It may be possible to automatically lift all supported operators through some clever application of the dot operators. Have to tinker a bit with this.

Copy link
Owner Author

Choose a reason for hiding this comment

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

possible, yes.. desirable? not sure. if we assume overflows and the like quit, this is not necessary really - one could build a special SafeInt type that would not quit for those that want their overflows served to them (such a SafeInt could also be efficiently implemented in a branchless fashion unlike the current throwing variant).. the safeint would probably not use Result but Optional more likely..

except:
R.err(getCurrentException())

template capture*(a: typedesc, e: ref Exception): Result[a, ref Exception] =
Copy link

Choose a reason for hiding this comment

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

Perhaps this API should be optimized to take advantage of the result symbol in the proc.

proc foo: Result[int, ref Exception] =
   if errorDetected:
      return captureStack() # this can figure out all the details from the `result` var.

Copy link
Owner Author

Choose a reason for hiding this comment

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

hm, as an overload perhaps - there's the case where you want to add some custom information to the error, or where you return the base class of a hierarchy of errors, but raise something more specific

Copy link

Choose a reason for hiding this comment

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

Yes, these short-cut forms could be just overloads.

Copy link
Owner Author

Choose a reason for hiding this comment

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

added a test for it

result.nim Outdated
else: self.value

iterator items[T, E](self: Result[T, E]): T =
## Iterate over result as if it were a collection of either 0 or 1 items
Copy link

@zah zah May 21, 2018

Choose a reason for hiding this comment

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

I'm not sure about this. It makes more sense for items to be lifted to iterate over the contents of T.

Copy link
Owner Author

Choose a reason for hiding this comment

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

these are nice to have when composing operations in a functional style - allows you to use generic code like any, map etc that operates on collections in some ways

Copy link

Choose a reason for hiding this comment

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

Yes, what I meant is that if you have a Result[seq[int] value, my expectation would be that I'm iterating over the integers in the sequence.

R.err("dummy")

proc works2(): R =
result.ok(42)
Copy link

Choose a reason for hiding this comment

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

perhaps ok and err can also take advantage of the result symbol to offer the short-cut form from Rust that you've grown to like:

template ok(value: typed) {.dirty.} = result.ok(value); return

proc works: R =
  if foo:
    ok 42
  else:
    err "dummy"

A bit too polluting perhaps as ok and err are pretty common terms, but it looks quite nice.

Copy link
Owner Author

Choose a reason for hiding this comment

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

mm... added a test to play with it - even like this, it's a half-baked emulation of what rust does though - there, Ok/Err can be used anywhere, even if the result is not a Result

doAssert $a == "Ok(42)"

doAssert a.mapConvert(int64)[] == int64(42)
doAssert a.mapCast(int8)[] == int8(42)
Copy link

Choose a reason for hiding this comment

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

All in all, looks pretty great.

For the intended support in handleErrors I imagined one more possible feature. There is a planned support in Nim for varargs generic types where the varargs parameter is mapped to a tuple (it's a relatively simple feature). We can then possibly add the option to instantiate Result with multiple possible error types (one of the goals of handleErrors is to make it easy to switch between enum errors and richer error types that can hold values).

Copy link
Owner Author

Choose a reason for hiding this comment

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

The question is though if that's really necessary.. ie if you want multiple error types, you can simply do a custom variant type as error and letting the result remain simple... if you want a tuple you simply set the error type to that tuple - the varags shortcut seems complicated for little benefit.

* remove default (string too heavy as default, no better alternatives
for now)
* restrict ok/err to Result types
* simplify signatures using implicit generics
* remove lifting example
* remove `items` from initial API
* add `ok` trick test using result type
* add `capture` trick test using resut type
* add `?` operator that exits early
@arnetheduck arnetheduck merged commit 01355f4 into master Aug 19, 2018
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