-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 optionals module (Maybe[T]) #2515
Conversation
I still think that the var x = y or z is better than: var x = y.get z When you want y or z depending if y is is nothing or not, regardless on what python does with it's dictionaries. Even better if the z is an Maybe too, as then the And will the Maybe module really be named "optionals"? |
Both of these options don't really belong in the language. But seeing as there is a precedent, I am inclined to prefer I took this opportunity to also replace
Even Haskell says:
I don't see a better name for the module. Besides, it is an opportunity to talk to more people in terms familiar to them: have both "optional" and "maybe" in the names. |
I've overhauled the module. The most important change: Now Basic usage remained almost the same. |
I'd like to object to this module for several reasons:
|
https://github.com/BlaXpirit/nre/commits/optionals
|
It is not compulsory to use this. First of all, one can specify concrete types like
The module lets you make a conscious choice. If you have a type that is not nullable, you can make it nullable to take advantage of Another important thing to notice is, these types are inevitably going to be present everywhere, even if it's through I really don't think it's possible to decide whether to include the boolean value or not inside the type declaration itself, thus the need for two types. I don't understand what problem your code snippet tries to solve. |
These would not be a problem if there was only a single Maybe[T] implementation type.
Same as above.
Please excuse the sloppy code, this is just some train-of-thought stuff: import macros
type
Foo = ref object
a: int
NonNilFoo = Foo not nil
Ref = concept x
isNil(x)
Nillable = concept x
isNil(x) is bool
x = nil
proc isOptimizable(T: typedesc): bool =
return T is Ref and T isnot Nillable
type Option[T] = object
val: T
when isOptimizable(T):
isSome: bool
proc isNillable(T: typedesc): bool =
when T is Ref and T isnot Nillable:
return true
else:
return false
proc box*[T](val: T): ref T not nil =
new result
result[] = val
doAssert false == isNillable(Foo)
doAssert true == isNillable(NonNilFoo)
doAssert false == isNillable(int)
doAssert false == isNillable(ref int)
type NNRI = ref int8 not nil
echo repr(Option[NNRI](val:box(1i8)))
echo repr(Option[int8](val:1, isSome: true)) # fails, isSome doesn't exist because of #2002 |
If it is possible to use |
Just to bikeshed a little: I prefer |
People seem to be in favor of dropping the nil-based implementation. So that's what I did... |
maybe.has | ||
|
||
|
||
proc val*[T](maybe: Maybe[T]): T = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too easy to use IMO, I'd like something long and verbose like "unsafeVal" better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with flaviut. People who want Maybe[T] also obviously want the extra typing.
LGTM 👍 A couple minor, non-blocking notes:
|
I prefer I would also prefer an optional default value for I dislike the |
## raises ``FieldError`` if there is no value. There is another option for | ||
## obtaining the value: ``val``, but you must only use it when you are | ||
## absolutely sure the value is present (e.g. after checking ``has``). If you do | ||
## not care about the tiny overhead that ``[]`` causes, you should simply never |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is using val
possible? It's not exported, so the docs shouldn't imply this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woops, forgot to change that.
We're slowly converging to https://github.com/flaviut/optional_t |
You're right. Perhaps we should just adopt it. |
I think that I like the 2nd ones better. By the way, "just" adopting optional_t is not so easy. In any case, I already have docgen docs and tests here. |
I like the second one better too. I am sceptical about the need for |
@dom96 I like an |
Alright, leave it in then. |
I like better the third option:
"a.get(b)" don't reads well in my opinion. I don't like much |
May I suggest to use the naming convention
The most concise naming pattern ( |
assert just(1) or just(2) == just(1) | ||
assert nothing(string) or just("a") == just("a") | ||
assert nothing(int) or nothing(int) == nothing(int) | ||
assert just(5) or 2 == 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this is weird. Traditionally, if we already have a value, injecting a default should not have any effect. I would expect just(5) or 2 == 5
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's getting parsed as assert just(5) or (2 == 2)
, although that should be a type error.
I'll look into it once I get to a computer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must be assert just(5) or (2 == 2)
. Don't forget converter toBool
. 😟
This is a huge argument against naming this operation as or
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. That's how it typechecks.
assert just(5) or 2 == 2
assert just(5) or (2 == 2)
assert toBool(just(5)) or (2 == 2)
assert true or true
Isn't it possible to just get rid of toBool()
? It's pretty much useless with ?=
anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converters are usually a bad idea. Remove the converter instead of renaming or
.
I spend a bit more time to make up my mind about the open questions here. @BlaXpirit I have made a few changes to this PR, which would be available here if you want to use any of this. What I changed, or was thinking about changing (apart from the name change mentioned above):
Sorry for the lengthy comment! |
Great points about About the names -- yes, I like some of those better as well:
Agree with everything except: In my view,
I don't see a need to cater for chaining of these calls because optionals are just going to be rarely used in Nim.
What else are you gonna use You're the first one to suggest an iterator. I don't see a need for it. It's weird. All in all, you're going too far seeking to borrow elegance from functional languages. Nim's standard library has neither the elegance, nor much else in common with them. |
I'd like that too.
👍
It's a container, it should not special case any type.
I dislike just a quick remark: for v in some(1):
... if v ?= some(1):
... Main advantage of |
The pull request started huge and complicated. Back then I wanted to have all the features one could possibly want. Then Araq shared his view. The main point of having this in the standard library is to avoid countless incompatible implementations. It is nice to be able to return an option from a function, instead of Then, if people insist, each individual additional operation's merits would be discussed separately, because there are just too many conflicting opinions here. Besides, it is easy to add more operations in your own code. Again, the main point is compatibility. |
In that case, all we need to get merged in this PR are the minimal operations for this type:
Note that I didn't include We can bikeshed over the rest later and figure things out. I'd also like to point out that this is a blocker for #2511, so the faster we get something merged, the faster we can move on to more exciting stuff. |
Dropping Maybe we should draw the line at "features that will obviously be useful all the time"? 😛 |
Nim has a huge potential to cater for the needs of functional programming as well. I don't see the benefit of leaving out common functionality. A functional programmer new to Nim will look for the "option monad", and may be disappointed that it is only semi-usable for their needs. With just a few additions, we could satisfy all typical use patterns of optionals in functional programming. @BlaXpirit Maybe a simple example helps to see why flatmapping is important and beautiful when it comes to option chaining. As soon "it is nice to be able to return an option from a function" it becomes "nice to easily combine these results". The question is not about whether or not optionals will be ubiquitous in Nim. The problem simply is there as well. The main point of having optionals is to provide a beautiful solution to the latter, not just wrapping a value. Regarding overloading |
Tell me when you guys figured it all out and want my review. ;-) IMO it should be as slim as possible and not support any FP feature under the sun just to show off that Nim can do it. |
This is pretty much what I tried to achieve today. I went back and forth from the interface definitions of Haskell/Scala/Rust, and reduced it to their bare minimum (e.g. Rust has more combination functions, which are somewhat uncommon anyways; Scala offers the whole "Traversable" stuff, which in Nim comes down to just providing an iterator, being the common denominator of all collections -- for that reason I also left out any "toContainer" conversion). In the end I added only 2 procs and 1 iterator (everything else was just renaming), so I really don't think that this is "going too far". Also, the names of the added functions ( |
Was just wishing that Nim had an Option/Maybe today! 👍 |
Adding extra functionality is easy, removing it is not. For this reason I think we need the slimmest implementation possible for now, until we can fully find out which functionality is missing and the best way to find that out is to start using this module. |
👍 for not having a converter and keeping it simple for the time being. Btw: I really think it should be just the
|
Closed because #2762 |
Superseded by #2762
This pull request is not final. I am looking for feedback.
I am planning to squash the commits at the end, but for now I will keep the history.