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

Optionals #2762

Merged
merged 6 commits into from
May 26, 2015
Merged

Optionals #2762

merged 6 commits into from
May 26, 2015

Conversation

flaviut
Copy link
Contributor

@flaviut flaviut commented May 19, 2015

Based heavily on #2515, but lots of stuff is ripped out and modified.

Contents:

  • The following procedures: some, none, isSome, isNone, get, unsafeGet, ==
  • The Option[T] type
  • tests using the unittest module
  • updated tutorial

This PR is purposely kept very simple in order to avoid bikeshedding and to get it merged. The reasoning is that new things can always be added in future PRs, removing functionality is much more difficult.

@flaviut
Copy link
Contributor Author

flaviut commented May 19, 2015

/cc @dom96 @Araq @bluenote10

@dom96
Copy link
Contributor

dom96 commented May 19, 2015

Ready to be merged as far as I'm concerned.

One small issue that I have is the use of FieldError, might be a good idea to introduce a custom exception object for this? (This can be done in the future easily though).

So yeah. Once @Araq is happy with it we can merge it.

@flaviut
Copy link
Contributor Author

flaviut commented May 19, 2015

I didn't notice the FieldError. I don't think that's a good idea as FieldError seems to be intended for use by the compiler. Perhaps a subtype of ValueError would be more appropriate?

@dom96
Copy link
Contributor

dom96 commented May 19, 2015

Yeah. Sounds good.

@dom96
Copy link
Contributor

dom96 commented May 19, 2015

I am also wondering whether we are now encouraging the use of typedesc as a param, none[int]() isn't as nice as none(int) but it is another thing to teach users of Nim. I suppose we were doomed into doing that as soon as this feature was implemented though so we may as well embrace it.

@flaviut
Copy link
Contributor Author

flaviut commented May 19, 2015

@dom96 good point re. typedesc. Maybe none[int] will work in the future, but none[int]() is the best solution for now.

@bluenote10
Copy link
Contributor

Yes, this is looking good. The only thing I would add is the implementation of $:

proc `$`*[T](o: Option[T]): string =
  ## Converts to string: `"some(value)"` or `"none(type)"`
  if o.isSome:
    "some(" & $o.val & ")"
  else:
    "none(" & T.name & ")"

From a foreign module, any attempt like this

import optionals
echo some(1)

currently leads to lib/system.nim(2068, 17) Error: undeclared identifier: 'val'. This is because it calls the generic implementation of $, which just iterates over fieldPairs without checking if the fields are public*.

Just out of curiosity: What is wrong with none(T: typedesc)? I didn't know that this is discouraged. I rather considered it already idiomatic because I have seen it frequently. It looks like having defined both none versions can cause trouble, so I would actually not provide none[T]() at all, and go for the nicer version right away?

(*) I came upon this issue before and noticed that a simple when compiles($value) within the loop fixes the problem. Good enough for a PR?

@dom96
Copy link
Contributor

dom96 commented May 20, 2015

(*) I came upon this issue before and noticed that a simple when compiles($value) within the loop fixes the problem. Good enough for a PR?

Please create a PR for this anyway. I wouldn't want this to get lost in the noise of this discussion.


I agree with @bluenote10, let's add $.

In regards to none(T: typedesc), I think it's more consistent with some[T](val) to use none[T]().

@bluenote10
Copy link
Contributor

In regards to none(T: typedesc), I think it's more consistent with some[T](val) to use none[T]().

It is inconsistent on call-site anyways, since nobody will write some[int](1). Imho some(1) and none(int) would match very well.

@dom96
Copy link
Contributor

dom96 commented May 20, 2015

hrm. Good point.

@flaviut
Copy link
Contributor Author

flaviut commented May 21, 2015

Ok, done.

@flaviut flaviut closed this May 23, 2015
@flaviut flaviut deleted the optionals branch May 23, 2015 14:30
@dom96
Copy link
Contributor

dom96 commented May 23, 2015

Why did you close this?

@flaviut flaviut restored the optionals branch May 23, 2015 15:35
@flaviut flaviut reopened this May 23, 2015
@flaviut
Copy link
Contributor Author

flaviut commented May 23, 2015

Looks like I accidentally deleted the branch in my repo. Sorry about that!

@reactormonk
Copy link
Contributor

So favor this PR over #2515?

@dom96
Copy link
Contributor

dom96 commented May 24, 2015

Yep.

@refi64
Copy link
Contributor

refi64 commented May 24, 2015

I liked the Maybe names better...

@oprypin
Copy link
Contributor

oprypin commented May 24, 2015

Please remove me as the author. Nothing that remains in this pull request was my idea.

By the way, I would hate using such a module. Absence of "get-or-default" is preposterous.

@dom96
Copy link
Contributor

dom96 commented May 24, 2015

@BlaXpirit We can add that later.

@flaviut
Copy link
Contributor Author

flaviut commented May 24, 2015

@BlaXpirit
Done. You are still listed as the author of 32ebcfa, but that's because those are your commits, squashed.

@oprypin
Copy link
Contributor

oprypin commented May 24, 2015

I disagree with some removals, but that's OK.
I just can't imagine any reason for removing `$`.

Probably time to merge.



proc unsafeGet*[T](self: Option[T]): T =
## Returns the value of a `just`. Behavior is undefined for `none`.
Copy link
Member

Choose a reason for hiding this comment

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

What's a "just"? ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Araq added a commit that referenced this pull request May 26, 2015
@Araq Araq merged commit 1ebff2e into nim-lang:devel May 26, 2015
@flaviut flaviut deleted the optionals branch May 26, 2015 13:47
@oprypin oprypin mentioned this pull request May 26, 2015
12 tasks
@ozra
Copy link
Contributor

ozra commented May 26, 2015

Ok, shedding time..
maybe sounds much better to me than "option". But regarding some/none vs Haskellish "just.." the 'some'-style sounds much better.

Get-or-default kind of is a must-have when juggling with maybes.

@flaviut
Copy link
Contributor Author

flaviut commented May 26, 2015

Get-or-default kind of is a must-have when juggling with maybes.

That's not really controversial. The controversial part is what exactly the syntax for them should be.

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.

8 participants