Implemented feedback for options

Author: PMunch

changelog.md

@@ -61,7 +61,7 @@
 
 - The options module has been mostly rewritten. Most things should work, but
   it's no longer possible to compare two ``none`` options. Use ``isNone``
-  instead.
+  instead. The comparator `==` is also gone, use `==?` instead.
 
 
 #### Breaking changes in the compiler

lib/pure/options.nim

@@ -28,12 +28,16 @@
 ## option returns an option.
 ##
 ## Comparing
-##   Comparing one option to another like ``some(100) == some(50)`` returns an
+##   Because of the way Nim treats comparators options uses special names to
+##   compare values. Every comparator is post-fixed with a question mark. So the
+##   equality operator becomes ``==?``, less than or equal to becomes ``<=?``
+##   and so on. This is also done to help alleviate some of the confusion.
+##   Comparing one option to another like ``some(100) ==? some(50)`` returns an
 ##   option. In this case they are not equal, so they return a ``none``. Note
 ##   however that it's not possible to compare two ``none`` values like this
-##   ``none(int) == none(int)`` as it returns a ``none``. In the case that a
+##   ``none(int) ==? none(int)`` as it returns a ``none``. In the case that a
 ##   comparator passes, the *first* option is returned so
-##   ``some(100) < some(200)`` returns ``some(100)``.
+##   ``some(100) <? some(200)`` returns ``some(100)``.
 ##
 ## Logic operations
 ##   Logic operations work on the options "has-ity" whether it has a value or
@@ -49,22 +53,6 @@
 ##   the one passed in. Since a ``none`` can't implicitly get a value ``not``
 ##   still returns a ``none`` when used on a ``none``.
 ##
-## Booleans
-##   Since both the comparators and the logic operators returns options it
-##   might be confusing to look at something like:
-##   ``if some(100) == some(100) == true`` but options will also implicitly
-##   convert to a boolean when used in that context (with it's has-ity as the
-##   value). So the above example could simply be written as
-##   ``if some(100) == some(100)``. But any value can also implicitly be
-##   converted to a ``some`` option. So we can simplify further with
-##   ``if some(100) == 100``, this works as 100 is converted to a ``some``
-##   which is then compared to the first ``some`` which returns a ``some(100)``
-##   that automatically get's evaluated to true since it has a value. Should
-##   the comparisson not hold we don't get an error but rather the comparator
-##   would evaluate to a ``none[int]`` and all ``none`` options evaluates to
-##   boolean false. Here we can see how treating the has-ity of an option makes
-##   a lot of intuitive sense.
-##
 ## Now that we have a better understanding of the nature of options we can take
 ## a closer look at how to use them.
 ##
@@ -157,14 +145,12 @@
 ## This means that we can insert checks in a stream of options:
 ##
 ## .. code-block:: nim
-##   assert (some(10) <= 20) == some(10)
-##   assert (some(30) <= 20).isNone
-##   assert (none(int) <= 20).isNone
+##   assert (some(10) <=? some(20)) == some(10)
+##   assert (some(30) <=? some(20)).isNone
+##   assert (none(int) <=? some(20)).isNone
 ##   let x = some(10)
-##   assert ((x >= 5) <= 20) == x
+##   assert ((x >=? some(5)) <=? some(20)) == x
 ##   # This only returns some if x is in the range 5..20
-##   # Note that since Nim currently rewrites >= to a <= statement with flipped
-##   # arguments this currently doesn't work as it returns some(5).
 ##
 ## Options can also be used for conditional chaining. Let's use the ``find``
 ## procedure we defined above:
@@ -186,10 +172,11 @@
 ## also be used in if statements:
 ##
 ## .. code-block:: nim
-##   if "hello world".find('w').?min(4) <= 2:
+##   if ("hello world".find('w').?min(4) <=? some(2)).isSome:
 ##     echo "Never run" # the first statement returns a none option, which when
 ##     # compared to 2 returns another none option. This none option is then
-##     # evaluated to false. In this case min is still never run.
+##     # obviously not a some. Note that in this case it would be better to use
+##     # a `match`.
 
 import typetraits
 import macros
@@ -267,10 +254,10 @@ proc get*[T](self: Option[T], otherwise: T): T =
     otherwise
 
 template either*(self, otherwise: untyped): untyped =
-  ## Wrapper around get with a default, for use as ``either(x, 20)``, if ``x``
-  ## is a ``none`` then it will return ``20``, otherwise it will return the
-  ## value of ``x``.
-  get(self, otherwise)
+  ## Similar in function to get, but if ``otherwise`` is a procedure it will not
+  ## be evaluated if ``self`` is a ``some``. This means that ``otherwise`` can
+  ## have side effects.
+  if self.isSome: self.val else: otherwise
 
 proc map*[T](self: Option[T], callback: proc (input: T)) =
   ## Applies a callback to the value in this Option
@@ -487,27 +474,29 @@ proc optCmp*[T](self: Option[T], value: Option[T],
   if self.isSome and value.isSome and cmp(self.val, value.val):
     return self
 
-template `==`*[T](self: Option[T], value: Option[T]): Option[T] =
+proc `==`*[T](self: Option[T], value: Option[T]): bool {.error.} = discard
+
+template `==?`*[T](self: Option[T], value: Option[T]): Option[T] =
   ## Wrapper for optCmp with ``==`` as the comparator
   optCmp(self, value, proc (val1, val2: T): bool = val1 == val2)
 
-template `not`*[T](self: Option[T]): Option[T] =
-  ## Not always returns a ``none`` as a ``none`` can't implicitly get a value
-  none[T]()
+template `!=?`*[T](self: Option[T], value: Option[T]): Option[T] =
+  ## Wrapper for optCmp with ``==`` as the comparator
+  optCmp(self, value, proc (val1, val2: T): bool = val1 != val2)
 
-template `<=`*[T](self: Option[T], value: Option[T]): Option[T] =
+template `<=?`*[T](self: Option[T], value: Option[T]): Option[T] =
   ## Wrapper for optCmp with ``<=`` as the comparator
   optCmp(self, value, proc (val1, val2: T): bool = val1 <= val2)
 
-template `>=`*[T](self: Option[T], value: Option[T]): Option[T] =
+template `>=?`*[T](self: Option[T], value: Option[T]): Option[T] =
   ## Wrapper for optCmp with ``>=`` as the comparator
   optCmp(self, value, proc (val1, val2: T): bool = val1 >= val2)
 
-template `>`*[T](self: Option[T], value: Option[T]): Option[T] =
+template `>?`*[T](self: Option[T], value: Option[T]): Option[T] =
   ## Wrapper for optCmp with ``>`` as the comparator
   optCmp(self, value, proc (val1, val2: T): bool = val1 > val2)
 
-template `<`*[T](self: Option[T], value: Option[T]): Option[T] =
+template `<?`*[T](self: Option[T], value: Option[T]): Option[T] =
   ## Wrapper for optCmp with ``<`` as the comparator
   optCmp(self, value, proc (val1, val2: T): bool = val1 < val2)
 
@@ -521,6 +510,10 @@ proc toOpt*[T](value: T): Option[T] =
   ## it's not already an option.
   some(value)
 
+template `not`*[T](self: Option[T]): Option[T] =
+  ## Not always returns a ``none`` as a ``none`` can't implicitly get a value
+  none[T]()
+
 template `or`*[T](x, y: Option[T]): Option[T] =
   ## When ``x`` is none, return ``y``
   let xx = x
@@ -536,14 +529,8 @@ template `and`*[T,U](x: Option[T]; y: Option[U]): Option[U] =
   else:
     none[U]()
 
-converter toSome*[T](value: T): Option[T] =
-  ## Automatic wrapping of values into ``some``
-  some(value)
-converter toBool*[T](opt: Option[T]): bool =
-  ## Options use their has-ity as their boolean value
-  opt.isSome
-
 when isMainModule:
+  echo some(10) < some(5)
   import unittest, sequtils
 
   suite "options":
@@ -576,12 +563,12 @@ when isMainModule:
       check none(string).isSome == false
 
     test "equality":
-      check some("a") == some("a")
-      check some(7) != some(6)
+      check (some("a") ==? some("a")).isSome
+      check (some(7) !=? some(6)).isSome
       check some("a").isNone == false
       check intNone.isNone
-      check (intNone == intNone) == false
-      check (none(int) == intNone) == false
+      check (intNone ==? intNone).isNone
+      check (none(int) ==? intNone).isNone
 
       when compiles(some("a") == some(5)):
         check false
@@ -591,7 +578,7 @@ when isMainModule:
     test "get with a default value":
       check(some("Correct").get("Wrong") == "Correct")
       check(stringNone.get("Correct") == "Correct")
-      check(either(some(100) < some(200), 10) == 100)
+      check(either(some(100) <? some(200), 10) == 100)
 
     test "$":
       check($(some("Correct")) == "Some(Correct)")
@@ -604,13 +591,13 @@ when isMainModule:
       intNone.map(proc (v: int) = check false)
 
     test "map":
-      check(some(123).map(proc (v: int): int = v * 2) == some(246))
+      check (some(123).map(proc (v: int): int = v * 2) ==? some(246)).isSome
       check(intNone.map(proc (v: int): int = v * 2).isNone)
 
     test "filter":
-      check(some(123).filter(proc (v: int): bool = v == 123) == some(123))
-      check(some(456).filter(proc (v: int): bool = v == 123).isNone)
-      check(intNone.filter(proc (v: int): bool = check false).isNone)
+      check (some(123).filter(proc (v: int): bool = v == 123) ==? some(123)).isSome
+      check some(456).filter(proc (v: int): bool = v == 123).isNone
+      check intNone.filter(proc (v: int): bool = check false).isNone
 
     test "flatMap":
       proc addOneIfNotZero(v: int): Option[int] =
@@ -619,26 +606,26 @@ when isMainModule:
         else:
           result = none(int)
 
-      check(some(1).flatMap(addOneIfNotZero) == some(2))
-      check(some(0).flatMap(addOneIfNotZero).isNone)
-      check(some(1).flatMap(addOneIfNotZero).flatMap(addOneIfNotZero) ==
-        some(3))
+      check (some(1).flatMap(addOneIfNotZero) ==? some(2)).isSome
+      check some(0).flatMap(addOneIfNotZero).isNone
+      check (some(1).flatMap(addOneIfNotZero).flatMap(addOneIfNotZero) ==?
+        some(3)).isSome
 
       proc maybeToString(v: int): Option[string] =
         if v != 0:
           result = some($v)
         else:
           result = none(string)
 
-      check(some(1).flatMap(maybeToString) == some("1"))
+      check (some(1).flatMap(maybeToString) ==? some("1")).isSome
 
       proc maybeExclaim(v: string): Option[string] =
         if v != "":
           result = some v & "!"
         else:
           result = none(string)
 
-      check(some(1).flatMap(maybeToString).flatMap(maybeExclaim) == some("1!"))
+      check (some(1).flatMap(maybeToString).flatMap(maybeExclaim) ==? some("1!")).isSome
       check(some(0).flatMap(maybeToString).flatMap(maybeExclaim).isNone)
 
     test "SomePointer":
@@ -697,8 +684,8 @@ when isMainModule:
     test "compare":
       proc leq(x, y: int): bool =
         x <= y
-      check optCmp(10, 20, leq) == some(10)
-      check optCmp(30, 20, leq).isNone
+      check (optCmp(some(10), some(20), leq) ==? some(10)).isSome
+      check optCmp(some(30), some(20), leq).isNone
 
     test "logical operators":
       let a = none[string]()
@@ -724,29 +711,29 @@ when isMainModule:
       check(sideEffects == 3)
       reset()
 
-      check ((genNone() or genSome()) == some("b")) == true
+      check ((genNone() or genSome()) ==? some("b")).isSome
       check sideEffects == 2
       reset()
 
-      check (((genNone() or genSome()) or genSome()) == some("b")) == true
+      check (((genNone() or genSome()) or genSome()) ==? some("b")).isSome
       check sideEffects == 2
       reset()
 
-      check ((genNone() or genSome()) or "c" == "b") == true
+      check ((genNone() or genSome()) or some("c")).unsafeGet == "b"
       check sideEffects == 2
       reset()
 
-      check ((genNone() or genNone()) or "c" == "c") == true
+      check ((genNone() or genNone()) or some("c")).unsafeGet == "c"
       check sideEffects == 2
       reset()
 
-      check (((genSome() or genNone()) or genSome()) or "c" == "a") == true
+      check (((genSome() or genNone()) or genSome()) or some("c")).unsafeGet == "a"
       check(sideEffects == 1)
       reset()
 
       # test ``and`` operator
 
-      check (((genSome() and genSome()) and genSome()) == some("c")) == true
+      check (((genSome() and genSome()) and genSome()) ==? some("c")).isSome
       check sideEffects == 3
       reset()
 
@@ -758,34 +745,34 @@ when isMainModule:
       check sideEffects == 2
       reset()
 
-      check isNone((genSome() and genNone()) and "c")
+      check isNone((genSome() and genNone()) and some("c"))
       check sideEffects == 2
       reset()
 
-      check (((genSome() and genSome()) and "c") == some("c")) == true
+      check ((genSome() and genSome()) and some("c")).unsafeGet == "c"
       check sideEffects == 2
       reset()
 
-      check isNone(((genNone() and genSome()) and genNone()) and "c")
+      check isNone(((genNone() and genSome()) and genNone()) and some("c"))
       check sideEffects == 1
       reset()
 
       # change the type during the expression and mix ``and`` with ``or``
-      check genNone() and 1 or 2 == 2 == true
-      check genSome() and 1 or 2 == 1 == true
+      check (genNone() and some(1) or some(2)).unsafeGet == 2
+      check (genSome() and some(1) or some(2)).unsafeGet == 1
 
     test "conditional continuation":
       when not compiles(some("Hello world").?find('w').echo):
         check false
-      check (some("Hello world").?find('w') == 6)
-      check (none(string) or some("hello corld") or some("hello world")).?
-        find('w') == -1
+      check (some("Hello world").?find('w') ==? some(6)).isSome
+      check ((none(string) or some("hello corld") or some("hello world")).?
+        find('w') ==? some(-1)).isSome
       var evaluated = false
-      if some("team").?find('i') == -1:
+      if (some("team").?find('i') ==? some(-1)).isSome:
         evaluated = true
       check evaluated == true
       evaluated = false
-      if none(string).?find('i') == -1:
+      if (none(string).?find('i') ==? some(-1)).isSome:
         evaluated = true
       check evaluated == false