Add a safe pattern and special forms for options #8358

PMunch · 2018-07-18T13:03:33Z

This adds the safer pattern of access from superfunc/maybe, along with adding the maybe logic defined in Toccata to avoid using boolean expressions alltogether.

mratsim · 2018-07-18T13:05:37Z

Can't we just overload case for Option?

mratsim · 2018-07-18T13:11:34Z

Or I like @dom96 suggestion on IRC to call it match

dom96 · 2018-07-18T13:14:28Z

Some discussion here: https://irclogs.nim-lang.org/18-07-2018.html#13:07:37

mratsim · 2018-07-18T13:16:45Z

Relevant: >>= name was rejected in PR #6404 for the flatmap name.

Also linked to RFC https://github.com/nim-lang/Nim/issues/7476

PMunch · 2018-07-18T13:27:47Z

Ah didn't know >>= was rejected elsewhere, that was just a copy from the implementation from @superfunc. And I don't think overloading case is an option as it's a builtin..

bluenote10 · 2018-07-18T15:06:14Z

It would be nice if optionCase (or whatever name it will be) is usable as both a statement and an expression. Is this the case? If so, the docs should clarify that (unless I missed it).

haltcase · 2018-07-18T17:46:09Z

Most of the IRC discussion seems on point — the logical macros are 🤷‍♂️ but the case one is pretty good. I dislike the names though and hope we can find something better.

This kind of thing is why I think a generic pattern matching construct would be so awesome, ideally in Nim itself as an upgrade to case or a new match expression. patty doesn't currently support a few important features like nesting, multiple patterns per branch, etc. Is this possible in userland and just a current limitation or is it something that needs to be more internal?

Varriount · 2018-07-19T06:02:29Z

lib/pure/options.nim

+    mVal = genSym(nskLet)
+    validExpr = newDotExpr(mVal, ident("isSome"))
+    valueExpr = newDotExpr(mVal, ident("unsafeGet"))
+    justClause = nnkStmtList.newTree(


Would using quote make this more readable?

Varriount · 2018-07-19T06:04:02Z

lib/pure/options.nim

+  )
+
+proc `>>=`*[T,U](self: Option[U], p: proc(value: U): Option[T]): Option[T] =
+  ## Used for chaining monadic computations together. Will create a none if


Could you add a runnable example for these procedures?

I've completely rewritten most of it now, and it's a lot clearer. Tried to add runnableExamples, but it messed up when using macros so I converted them to code-blocks for now.

andreaferretti · 2018-07-19T07:17:45Z

@citycide Neither nesting nor multiple patterns per branch are an instrinsic limitation. In fact, everything that is listed on the README of Patty as currently missing is doable with macros only. It would also be nice to include this macro to support options.

It is just that I could not find the time to work on these other features. Nesting is probably the main one, because it requires the macro to become recursive. After this restructuring is done, adding different kind of base patterns, such as options, arrays and seqs, should be doable easily

PMunch · 2018-07-19T23:00:55Z

Okay, I've completely rewritten most of this now. It has a match macro for case-based pattern matching, it has a conditional continuation operator .? that works both for values and in ifs. And it has a complete set of comparators and logic operators defined for options.

There is one issue in that Nim refuses my overload of >= and simply rewrites it to <= while flipping the argument. This completely breaks the logic of comparators on options. If it's not made possible in the compiler to do this then we must simply rename them to something else. Maybe >=?, <=?, ==? etc.?

krux02 · 2018-07-20T00:08:22Z

lib/pure/options.nim

+      reset()
+
+      check genNone() or genNone() or "c" == "c" == true
+      check sideEffects == 2


can you put braces () here? I have no idea how the AST is constructed with the two == operatiors. The or operator has lower operator precedence that ==. This is what is confusing me. And please also with the other tests.

EDIT:

according to the parse tree, this is the AST: check((genNone() or genNone()) or (("c" == "c") == true))) are you sure you want to test that?

Just updated now

andreaferretti · 2018-07-20T07:49:08Z

Just wanted to mention that using match for pattern matching will collide with patty. Of course patty has lower precedence than stdlib when it comes to naming.

Nevertheless, there is an overlap here - options may be just another pattern that patty recognizes.

What should we do?

[ ] Rename match in patty and have two different forms of pattern matching for options vs anything else
[ ] Include option pattern matching in patty
[ ] Deprecate patty and include other forms of pattern matching in stdlib
[ ] Other?

mratsim · 2018-07-20T08:28:52Z

Same remark as #8369:

or and and are better than optOr and optAnd, but due to potential confusion for Option[bool], I would like a different name for option combinators. Rust uses and_then and or_else for example.

PMunch · 2018-07-20T08:37:26Z

@mratsim, it's not possible without modifying the internals though to create new non-symbol operators.. Having some confusion for Option[bool] is in my opinion much better than user a regular call operator.

@andreaferretti, we can change the name if people want. match just seemed to be the consensus before my latest changes.

andreaferretti · 2018-07-20T08:58:57Z

@PMunch I have nothing against match, but if we introduce pattern matching for options in stdlib it may make sense to introduce a more general construct

PMunch · 2018-07-20T09:15:31Z

Ah, that makes sense. This isn't quite pattern matching though. It is just a wrapper around an if and a case statement. All the actual matching is done by the case statement, which I guess is already a general construct.

match ourOption:
  some _ of 0..high(int): echo "It's huge!"
  some x: echo "It's a measly ", x
  none: echo "It's nothing at all"

would get converted to:

let tmp = ourOption
if tmp.isSome:
  case tmp.val:
  of  0..high(int):
    echo "It's huge!"
  else:
    let x = tmp.val
    echo "It's a measly ", x
else:
  echo "It's nothing at all"

andreaferretti · 2018-07-20T09:18:02Z

@PMunch well, that's exactly what patty does - rewrite into a case statement - but for other types of values otehr than options (objects and variant objects)

PMunch · 2018-07-20T09:36:17Z

@andreaferretti ah interesting, I'm not really qualified to answer your question though. I'm happy as long as we get a safe and easy pattern for options. The best case would of course be if we could shadow an identifier as well. So if a user tried to do something like this:

let noneOpt = none(int)
match noneOpt:
  some _: echo noneOpt.get
  none: echo noneOpt.get

It would actually throw an error instead of compiling and later throwing a runtime error.

@bluenote10 forgot to answer this. The matching didn't support that when you asked, but they do now.

Varriount · 2018-07-20T10:03:17Z

lib/pure/options.nim

@@ -139,6 +266,12 @@ proc get*[T](self: Option[T], otherwise: T): T =
  else:
    otherwise

+template either*(self, otherwise: untyped): untyped =


Since this takes an untyped statement, does it follow short-circuit logic? That is, if self is none, otherwise won't be evaluated?

Ah, not currently. It's just a wrapper for get with a default value. But I think it should be changed to do that instead. Makes more sense when seen along with or and and.

Varriount · 2018-07-20T10:04:23Z

lib/pure/options.nim

-  ## the type contained in the option.
+  ## the result will be `Some(x)` where `x` is the string representation of the
+  ## contained value. If the option does not have a value, the result will be
+  ## `None[T]` where `T` is the name of the type contained in the option.
  if self.isSome:
    "Some(" & $self.val & ")"


Will this work properly with strings? I seem to recall that addQuoted might need to be used in these cases.

@Varriount that is a true criticism. But this is a problem unrelated to this PR.

Hmm, not entirely sure. That is one of the few things I haven't changed in this module. Only change I did there was to change the wrapping of the comment to observe 80 chars.

Varriount · 2018-07-20T10:05:37Z

lib/pure/options.nim

+  ## If nothing is returned from ``statements`` this returns nothing.
+  ##
+  ## .. code-block:: nim
+  ##   echo some("Hello").?find('l') ## Prints out Some(2)


I believe these should go on a runnableExamples block.

Yeah, I originally had them as runnableExamples but it got confused and produced some really weird code from it that failed spectacularly. Try and change them and you'll see.

Varriount · 2018-07-20T10:05:47Z

lib/pure/options.nim

+  ## Basic pattern matching procedure for options. Converts the statements to a
+  ## check and a ``case`` statement. The structure of the matching body is as
+  ## follows:
+  ## ..code-block:: nim


I believe these should go on a runnableExamples block.

Varriount · 2018-07-20T10:07:35Z

lib/pure/options.nim

+  #echo result.repr
+
+proc optCmp*[T](self: Option[T], value: Option[T],
+  cmp: proc (val1, val2: T): bool): Option[T] =


Should there be an additional optCmpIt proc, mirroring the *It procedures in sequtils.nim?

Yeah that would be nice.

krux02

Generally I have to say, I don't like this PR at all. I am sorry to say this, because you probably invested a lot of time in it. I can see that, but overall it feels lake a big hack that tries to make everything compile to something. It really circumvents the safety net that static typing normally brings.

krux02 · 2018-07-20T10:17:25Z

lib/pure/options.nim

-  ## the type contained in the option.
+  ## the result will be `Some(x)` where `x` is the string representation of the
+  ## contained value. If the option does not have a value, the result will be
+  ## `None[T]` where `T` is the name of the type contained in the option.
  if self.isSome:
    "Some(" & $self.val & ")"


@Varriount that is a true criticism. But this is a problem unrelated to this PR.

krux02 · 2018-07-20T10:19:33Z

lib/pure/options.nim

+  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


WTF, what are you thinking here?

It started as a workaround in that x != y get's rewritten to not x == y which breaks the logic of maybes. But when you think about it what would the not of a none be? You can't create a some out of it as that would imply it has a value..

how about == should not return an option in the first place? It should return a bool, always. You create a situation where both x == y and x != y both can have the same value.

how about this:

template `not`*[T](self: Option[T]): bool = self.isNone

You don't really need to keep the type T, because the value is either thrown away, or you need to come with something that is true.

i think that entire idea of not(Option[T]) smells bad. If you go this way, please also redefine and/or. And move it into separate module. Separate of stdlib. Main options.nim should return "==" result as bool, and avoid extending standard language functions with new meanings. Such level of changes to the language should be done after thorough discussion rather than on the spot.

krux02 · 2018-07-20T10:20:41Z

lib/pure/options.nim

+
+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)


this is an unnecessary lambda expression. Lambda expressions have a bad performance penalty, because they can never be inlined. Comparison is something that is very likely to benefit from inlining.

How would this be written then? I just went with the first thing that worked..

There are so many problems here. I think the biggest problem is that == returns an Option type. It shouldn't. And for performance, you just inline the comparison. You can do it with a template if you want code duplication.

if self.isNil: value.isNil else value.isSome and self.val == value.val

krux02 · 2018-07-20T10:30:32Z

lib/pure/options.nim

+      check sideEffects == 2
+      reset()
+
+      check ((genNone() or genSome()) or "c" == "b") == true


this should be a compile error
((genNone() or genSome()) or ("c" == "b")) == true
here you resolve the operator proc `or`(x:Option[string]; y: bool). This operator should not exist, because the result type is unclear.

Another reason why this should not compile is. It it would not have compiled, you would have noticed this unwanted behaviour. Don't try to make every expression valid somehow. When you do that it will be very hard to find unwanted behavior.

Yeah that's a fair point. The problem here is that options are implicitly boolean and values are implicitly some to make them more ergonomic to use. As I mentioned in my first comment after the rewrite I think the comparators need a different symbol, maybe ==?, !=?, etc. That would also make this fail as you would use ==? to compare the two "c"-s

I have an idea. How about you implement all these operators in a nimble module. There you can experiment as much as you want without stepping on anybodys foot. Nim really doesn't need you to have write acces to the options module to exend that type with behavior.

krux02 · 2018-07-20T10:32:20Z

lib/pure/options.nim

+      check sideEffects == 2
+      reset()
+
+      check ((genNone() or genNone()) or "c" == "c") == true


I could have written it above, but here it is equally true. Why would you want to ever test _ == true? In my opinion that should never be necessary or meaningful.

It makes more sense if you consider that or and == are overloaded here. If they had different names it would be more obvious.

The right operator is of type boolean, so the reader expects that the left operand is also of type boolean and that there is absolutely no overload happening. But you just break that assumption and convert the right operator with a conversion proc into an option type. This is one reason, why converter procs are a bad thing. They hide what is actually going on in the code.

krux02 · 2018-07-20T10:41:49Z

lib/pure/options.nim

+    test "conditional continuation":
+      when not compiles(some("Hello world").?find('w').echo):
+        check false
+      check (some("Hello world").?find('w') == 6)


this should not compile. some(6) and 6 are different types. They are not equal, and therefore should not be equal.

All values are implicitly a some from the converter, which is why this works.

andreaferretti · 2018-07-20T11:46:25Z

lib/pure/options.nim

+
+converter toSome*[T](value: T): Option[T] =
+  ## Automatic wrapping of values into ``some``
+  some(value)


I don't think using converters is a good idea here. T and option[T] are different for good reasons, I prefer excplitly writing some(x) if necessary

Yeah I wasn't sure about that either and it seems to already cause some confusion.. The idea was to reduce the effort of using options but I guess it might be better to err on the side of being explicit here.

andreaferretti · 2018-07-20T11:46:55Z

lib/pure/options.nim

+    y
+  else:
+    none[U]()
+


Uh? What does this operation mean?

It's explained in the "Logic operations" part of the tutorial. and and or return an option when all, or one of it's arguments is an option respectively. and requires both options to be some and returns the last one, or none if one them isn't a some. or requires one of the options to be some and returns the first one that is. This means you can implement full boolean logic through only combining options.

I understand what these operations do. I don't understand what they mean, unlike - say - their boolean counterparts.

andreaferretti · 2018-07-20T11:47:39Z

lib/pure/options.nim

+  ## Procedure with overload to automatically convert something to an option if
+  ## it's not already an option.
+  some(value)
+


There is some already, isn't this redundant?

No, it has two versions, one that take an option and one that doesn't. This means that if the value is an option it will not be wrapped in a second option, but if it's not an option it will be converted into one.

Nim is statically typed, so I don't see in which situation you may not know whether you already have an option or not

@andreaferretti Nim also supports generic programming (including templates), where you may need to support both types with the same code

andreaferretti · 2018-07-20T11:48:07Z

lib/pure/options.nim

+proc toOpt*[T](value: Option[T]): Option[T] =
+  ## Procedure with overload to automatically convert something to an option if
+  ## it's not already an option.
+  return value


Why is this needed?

See my other comment

andreaferretti · 2018-07-20T11:49:19Z

lib/pure/options.nim

+converter toBool*[T](opt: Option[T]): bool =
+  ## Options use their has-ity as their boolean value
+  opt.isSome
+


Same here, people can use isSome explicitly, no need to circumvent type safety

Yeah it seems like this is just causing confusion so I think it's best to just remove the converters.. Or at least hide them behind a flag..

andreaferretti · 2018-07-20T11:53:20Z

lib/pure/options.nim

@@ -139,6 +266,12 @@ proc get*[T](self: Option[T], otherwise: T): T =
  else:
    otherwise

+template either*(self, otherwise: untyped): untyped =


In many languages Either is another data type, related to Option. Using either here can be confusing, and it seems not necessary, since it is just an alias for get

@krux02 asked if this would evaluate other if it was a procedure, and I think that it makes sense if it's rewritten to not do that and keep both either and get with default. This means you can do `either(, procedureWithSideEffects()) and only if nothing comes of the options statement then will the side effects procedure be run.

andreaferretti · 2018-07-20T11:59:11Z

I have to say I am not very convinced by this PR. It seems to add 4 things:

some converters (A -> Option[A], Option[A] -> bool, ...) which I feel are only detrimental to type safety
some utility function. I like some of them (map, filter, or, flatMap, flatten) but others seem to be there just because of similarity with boolean, even if they don't make much sense (and, not)
a macro .? for chaining access to fields that may be options - I like this
a macro match for pattern matching - I also like this, but I feel that a more general one is needed

My suggestion would be to split this PR into smaller ones that can be discussed independently - I wouldn't like to accept this in toto

PMunch · 2018-07-20T12:12:04Z

@andreaferretti, yeah I think the converts will have to go. They seem to cause more trouble than what they're worth. The utility functions you seem to like are already in the options module, not something that I've added. The and/not/or and comparators are new, and they create a powerful set of tools to work with options, but I feel that the comparators should change name. I would like to do the same with or and and but unfortunately we can't create new textual operators without modifying elsewhere. The match macro is just a wrapper over the more general concept case, but I guess the name might be a bit misleading. Not quite sure what else to call it though, people didn't see to like the original optionsCase either.

Araq · 2018-08-01T16:44:16Z

Well that arrow notation seems completely arbitrary to me. At least come up with a great syntax please when you push for this non-idiomatic feature. ;-)

mratsim · 2018-08-02T12:47:06Z

Let's start with pattern matching for case as everyone seems to agree on it.

Then we can think about more syntactic sugar. I like if let because no symbol that are hard to search, and Nim assignation is let, not <-.

andreaferretti · 2018-08-02T13:31:47Z

I, for one, do not agree in having pattern matching for case. Not that I would dislike it, but doing it with a macro is a great opportunity to show the extensibility of Nim.

It would be really nice to have some form of pattern matching - more general than patty - in the standard library as an importable macro, rather than baking it into the compiler

mratsim · 2018-08-02T16:08:44Z

@andreaferretti, iirc the goal is for Araq to add caseMatchStmt which similar to forLoopStmt currently, can allow macros to work on case statement.

See this example for forLoopStatement:

macro enumerate(x: ForLoopStmt): untyped =
  expectKind x, nnkForStmt
  # we strip off the first for loop variable and use
  # it as an integer counter:
  result = newStmtList()
  result.add newVarStmt(x[0], newLit(0))
  var body = x[^1]
  if body.kind != nnkStmtList:
    body = newTree(nnkStmtList, body)
  body.add newCall(bindSym"inc", x[0])
  var newFor = newTree(nnkForStmt)
  for i in 1..x.len-3:
    newFor.add x[i]
  # transform enumerate(X) to 'X'
  newFor.add x[^2][1]
  newFor.add body
  result.add newFor

for a2, b2 in enumerate([1, 2, 3, 5]):
  echo a2, " ", b2

Then anyone can metaprogram the case statement for their own types. There is a real need to resolve type though like macro case[Option](x: caseMatchStmt[Option]): untyped = ...

krux02 · 2018-08-02T21:03:42Z

@mratsim Macros do work on case statements.

dom96

So I think a lot of the discussion here has focused on pattern matching so far. This PR does a lot more than just that, and as it stands I would reject all of it except the .? macro.

Can we close this PR and create a new one with this macro? We can then discuss pattern matching in a separate issue/forum thread (I strongly suggest the latter).

krux02 · 2018-08-03T14:31:41Z

@dom96 just close it.

PMunch · 2018-08-04T20:37:26Z

Oh wow, I've been away for two weeks on vacation and there are a lot of replies to go through. I'm still polishing my article on why this way of treating options is a good idea. But it's mostly about the concepts, the usage of or, and, or any other names doesn't really matter for that. But they do mean that it's more ergonomic to use them (in my opinion). When it comes to the matcher it really was just something I threw together as it wasn't all that much extra work. The most important part of it is the two distinct branches making it safer to use options. I'll read through more of these comments tomorrow and try to finish up my article on options, it will hopefully make a more coherent argument than all these comments.

PMunch · 2018-08-06T20:17:39Z

I've now finished up my article on options, and why I think adding all of these are a good idea: https://peterme.net/optional-value-handling-in-nim.html

andreaferretti · 2018-08-07T08:43:42Z

I have read @PMunch article, and I am still unconvinced.

For one, the reason for introducing and for options relies on the similarity to how bash handles contructs like do_this && do_that. Now in bash it is cumbersome to do otherwise, os one relies on and being short-circuiting to execute side effects conditionally. Personally I consider this to be a hack and far less readable than explicit control flow (yes, with an if).

About match, I am still of the same opinion: it is a useful construct, but I would not want it implemented in the standard library in a way that only works for options.

Finally, the comparators like <? may be nice in some contexts but seem definitely a niche thing. By the same token, one may want to extend every predicate to options - I do not see why < or > should be given special treatment. I think the best place to implement this DSL would be a Nimble package of its own.

So in the end I still think that the best course of action is to extract the .? macro as a PR for sugar.nim and implement the rest of these operations inside a Nimble package.

PMunch · 2018-08-07T09:46:23Z

The implementation of or and and is similar to how they work in bash, but that is not the reason for implementing them here. In bash everything returns a return code, and writes what in other languages would be the return value to a stream. This return code works like an option with && and || to allow conditional chaining of commands. But since it only works on return codes in bash it is far less powerful than the general concept. Probably the simplest task that shows some of that power is the default value for lookups:

let name = userTable.getOpt("name") or some("John Doe")

This could of course be done with inline if statements in Nim:

let name = if userTable.hasKey("name"): userTable.get("name") else: "John Doe"

But in my opinion this is a bit on the verbose side, and it shows the check-blindness concept I mentioned in the article. If I mess up and put the get statement on the wrong side I will get a guaranteed run-time error that the compiler won't be able to catch. Sure, it is my fault for writing stupid code, but there simply isn't a way to write this kind of stupid code when using options.

Now that does little more than what the proposed either template would do, except it returns an option, making it composable so we could do something like this:

let name = userTable.getOpt("name") or settings.getOpt("defaultName")
echo either(name, "No name found and no default name defined")

Or if we wanted some error checking and wanted to use the matcher:

let name = userTable.getOpt("name") or settings.getOpt("defaultName")
match name:
  some x:
    echo "Welcome ", x
  none:
    echo "No name found and no default name defined"

I share your concern with match but I think that can be addressed by a name change until we have a more robust solution. It is an integral part of working with options this way as it assures that you can only use the value of the option if you have actually made sure that it exists.

The scepticism around the comparators is also fair, they are more of a niche feature and as I mention in the article they are more there to flesh out the concept and make it a bit easier to work with options. I implemented optCmp which took an arbitrary comparator, but I noticed I wasn't able to pass ==, !=, <=, etc to it so I opted to make those special in order to avoid the user having to wrap them in a lambda if they wanted to use them.

andreaferretti · 2018-08-07T12:18:12Z

Just a small note: you may have not noticed I have issues with and and not. You explain in detail the use of or, but I am completely fine with or (it is a common operation found in many other languages - for instance that would be orElse in Scala)

PMunch · 2018-08-07T15:49:16Z

Yeah I agree that not should be removed, it really has no use-case I can think of. and on the other hand is essentially the same as && in bash, whether it's useful really depends on the context of the has-ity. If you return an option with an error code (similar to what bash does) then and often makes more sense than or as you essentially want to continue if the option doesn't have a value. Plus having only one or the other would be really strange as they are two halves of the same concept.

jduey · 2018-08-09T17:30:18Z

Hi folks. I'm the BDFL of Toccata. I'm gobsmacked that anyone would think an idea of mine is interesting enough to include in another language. And your discussion here has given me some food for thought. I'm not that knowledgeable about Nim, but I've been intrigued by it for some time. Happy to answer any questions anyone has about Toccata, though. :)

Araq · 2018-08-09T18:00:51Z

@jduey Can you please explain to me why bool loses information but int does not? No offense but it seems completely arbitrary to me. Most programming languages lack units of measure which seems to be the "real" problem you are concerned about.

jduey · 2018-08-09T18:22:50Z

I can try. I don't see it as 'int' not losing any info as much as 'bool' loses all info except for a single bit. An 'int' still has a magnitude and 32 or 64 bits of info. If a programmer wants an int to carry more info, such as units, they can wrap it in a type.

There's also a question of how big a step to take at once. Eliminating bools was a small enough step I could feel confident in taking it. Eliminating unadorned ints is a much bigger step. And now that you mention it, I might have to put some thought into that.

PMunch · 2018-08-09T19:59:18Z

Oh hi @jduey, nice to see you here :)

@Araq, I mentioned this in my article. IMHO boolean-blindness is, as you mention, a bit silly. All values lose information, it's not special for booleans. I also link to another article that points this out and generalizes the problem to something he calls part-blindness. This concept is however a bit too broad and is dealt with in Nim in other ways. But back-tracking to boolean blindness most people tend to talk about it when performing checks. Things like:

if x.isNil:
  echo x[]
else:
  echo "X is nil!"

Now this obviously won't work, as I've switched the if bodies around by "accident". This is what I refer to as check-blindness in my article, the fact that the context of the boolean check is lost. By using options and the special case .? and match the context becomes part of the block and the value can only be extracted when it is valid. This means that the above statement becomes something like this:

match some(x) !=? some(nil):
  some y: echo y[]
  none: echo "x is nil!"

This shows that the context of the check is not lost, it lives alongside the option, and the value we care about can only be used when it is valid.

PS: I'm planning on adding overloads to the postfixed comparators and the right hand side of the and and or to avoid many of the some(x) statements you now need. This would eliminate the two in the above statement.

PMunch · 2018-08-14T15:18:18Z

Okay, did some more rewrites. Now you can use all the logic operators and comparator operators with the right-hand value being a value and not an option. Just means you don't have to put some everywhere in your code. Also added some simple wrapper macros that will auto-wrap non-option procedures.

match is now gone, say hello to require! require doesn't turn everything into a case statement, and can't do the value matching thing, that can be done elsewhere. What require does is take in one statement, or a list of statements, and evaluates them one by one until one returns a none. If none of them return a none it will run the just branch and allow you to map the different values to symbols. Something like this:

require ["abc".find('o'), "def".find('f')]:                               
  some [firstPos, secondPos]:                                             
    echo "Found 'o' at position: ", firstPos, " and 'f' at position ",    
      secondPos                                                           
  none: echo "Couldn't find either 'o' or 'f'"

IMO this is more convenient than match and hopefully not as controversial.. Another option would of course be to move this to a new module optionutils, that way they are a bit more opt-in.

dom96 · 2018-08-14T16:39:50Z

Sorry, but I don't understand the rationale for this complete rewrite. Hundreds of lines of code are using get and you're deprecating it here for seemingly no compelling reason.

I'm happy to accept a pattern matching macro (and the .? operator), but the rest should stay as it is. At this point you might just be better off creating a new PR for these two things (and I would encourage creating separate PRs)

PMunch · 2018-08-14T17:14:57Z

What? This isn't a complete rewrite.. I had to force push over my copy after I rebased against the current devel. The deprecation has been there from the start (or at least close to it).. Could of course remove it, but since no-one mentioned it I thought people were fine with it. The reason to deprecate it is to let people use the more powerful constructs that avoids silly runtime errors.

As I mentioned in my last comment I removed the pattern matching part of match and renamed it to require and now it only does the unpacking of options when they have a value.

What about the new wrapping macros? Yay or nay? And what don't you like with the comparators and logic operators?

PMunch · 2018-08-14T17:43:18Z

Got tired of discussing this since it was basically just the same couple arguments over and over. So I removed everything but require, .?, and the new wrappers. Also removed the deprecation warnings and added back in the old ==. Hopefully this is less scary and will actually get added :)

PMunch · 2018-08-17T09:44:05Z

Split options into options and optionutils and moved all the map, flatMap, .?, etc. to the new module. Also changed the name from require to allSome per Araqs request.

PMunch · 2018-08-17T11:14:15Z

Had to un-fuck the git history of this PR. I made some mistakes when trying to make it up to date with devel so it would merge properly.

PMunch · 2018-10-03T09:55:45Z

I've now split this into four separate PRs as requested: #9160, #9161, #9162, #9163. Please move discussion to the corresponding PRs.

Varriount requested changes Jul 19, 2018

View reviewed changes

krux02 mentioned this pull request Jul 19, 2018

logical operators for option type #8369

Closed

krux02 reviewed Jul 20, 2018

View reviewed changes

PMunch mentioned this pull request Jul 20, 2018

Fix issues for procedures with side effects and sym vs. ident superfunc/maybe#10

Merged

Varriount requested changes Jul 20, 2018

View reviewed changes

krux02 reviewed Jul 20, 2018

View reviewed changes

andreaferretti reviewed Jul 20, 2018

View reviewed changes

dom96 requested changes Aug 3, 2018

View reviewed changes

PMunch force-pushed the safeoptions branch from 120691f to aa64506 Compare August 14, 2018 14:11

PMunch force-pushed the safeoptions branch from 93fa76e to 070847c Compare August 17, 2018 10:59

Split options into options and optionutils

606544d

PMunch force-pushed the safeoptions branch from a858c0b to 606544d Compare August 17, 2018 11:13

PMunch mentioned this pull request Oct 3, 2018

Split options module into options and optionsutils #9160

Closed

PMunch closed this Oct 3, 2018

Add a safe pattern and special forms for options #8358

Add a safe pattern and special forms for options #8358

Conversation

PMunch commented Jul 18, 2018

mratsim commented Jul 18, 2018

mratsim commented Jul 18, 2018

dom96 commented Jul 18, 2018

mratsim commented Jul 18, 2018 • edited Loading

PMunch commented Jul 18, 2018

bluenote10 commented Jul 18, 2018

haltcase commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaferretti commented Jul 19, 2018

PMunch commented Jul 19, 2018

krux02 Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaferretti commented Jul 20, 2018

mratsim commented Jul 20, 2018

PMunch commented Jul 20, 2018

andreaferretti commented Jul 20, 2018

PMunch commented Jul 20, 2018

andreaferretti commented Jul 20, 2018

PMunch commented Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krux02 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krux02 Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaferretti commented Jul 20, 2018

PMunch commented Jul 20, 2018

Araq commented Aug 1, 2018

mratsim commented Aug 2, 2018

andreaferretti commented Aug 2, 2018 • edited Loading

mratsim commented Aug 2, 2018 • edited Loading

krux02 commented Aug 2, 2018

dom96 left a comment

mratsim commented Jul 18, 2018 •

edited

Loading

krux02 Jul 20, 2018 •

edited

Loading

PMunch commented Jul 20, 2018 •

edited

Loading

krux02 Jul 20, 2018 •

edited

Loading

andreaferretti commented Aug 2, 2018 •

edited

Loading

mratsim commented Aug 2, 2018 •

edited

Loading

andreaferretti commented Aug 7, 2018 •

edited

Loading

PMunch commented Aug 9, 2018 •

edited

Loading

PMunch commented Aug 14, 2018 •

edited

Loading

dom96 commented Aug 14, 2018 •

edited

Loading