Skip to content

Add allSome macro to options which safely unwraps options #9162

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

Closed
wants to merge 4 commits into from

Conversation

PMunch
Copy link
Contributor

@PMunch PMunch commented Oct 3, 2018

This is based on the safe pattern for unwrapping options found in https://github.com/superfunc/maybe but is extended to allow not only a single option, but multiple. It's also made safe from side-effects.

allSome accepts a list of options (or a single option), and two blocks. One in which all the options are unpacked, the other in which one or more of the options doesn't have a value. This ensures that no safe unpacking of options occurs, and allows the user to check multiple options in a simple way.

Depends on PR #9160

Copy link
Collaborator

@mratsim mratsim left a comment

Choose a reason for hiding this comment

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

Either name is good, Reading some of the examples of allSome it kind of read weirdly, maybe allMatch instead?

Also those code-blocks should be runnableExamples

## let x = allSome(["abc".find('b'), "def".find('f')]):
## some [firstPos, secondPos]: firstPos + secondPos
## none: -1
## echo x # Prints out "3" (1 + 2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those should probably be runnableExamples

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Last I tried this it didn't work as runnableExamples. That issue might've been fixed by now though so I'll check it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appears to work, although I do have to add in import options and an implementation of the find procedure that returns an option.

none(T)
else:
self
template either*(self, otherwise: untyped): untyped =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the name

else:
self

macro allSome*(options: untyped, body: untyped): untyped =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe allMatch? reading some of the examples is a bit awkward

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was Araq's latest suggestion. Problem is that it doesn't really do "matching" in the grand sense of the word, it just checks the has-ity of the options..

@superfunc
Copy link
Contributor

This is so neat, thanks for pushing it forward @PMunch :)

@PMunch
Copy link
Contributor Author

PMunch commented Oct 3, 2018

Thanks @superfunc, do you have any naming suggestions?

@superfunc
Copy link
Contributor

Naming is hard haha, I can't think of a better one.

@alehander92
Copy link
Contributor

I am not entirely sure if a special macro is needed for this: a general enough pattern matching library should be able to do it. E.g. in gara it should be almost possible(needs a tuple support fix) to define an unpacker allSome and do

match (e, f, g): # options
of allSome(@e1, @f1, @g1):
  echo e1
else:
  echo 0

On the other hand if we need allSome in the stdlib, there is no other way but a special-case macro I guess

@PMunch
Copy link
Contributor Author

PMunch commented Oct 6, 2018

Problem is there is no pattern matching library in the stdlib and now that nilseqs and nilstrings are removed from the language and options take over their functionality I feel like we need a better way to handle options.

@dom96
Copy link
Contributor

dom96 commented Oct 6, 2018

Can't we put this in a Nimble package for now and see what the adoption looks like?

I wasn't around for the discussion relating to splitting options into two modules, but I dislike this.

@PMunch
Copy link
Contributor Author

PMunch commented Oct 8, 2018

There already is a similar Nimble package which I've based this on since I felt like it would be a natural part of the stdlib.

The splitting of options was a request from Araq, I've got no issue reverting it if he's changed his mind.

You've said yourself that options are clunky and impractical to use, this fixes the problem in a safe and elegant way. With options now presumably being used more, or at least being made more visible to people as it's mentioned each time nilstring and nilseqs are mentioned, I felt like there was a need to make them more practical. I'm using the original package in a project I'm working with right now and it's a lot better than using the current Nim stdlib way.

@alehander92
Copy link
Contributor

@PMunch I agree, probably there isn't gonna be pattern matching in the stdlib soon.
It's a chicken-and-egg problem.

I am just afraid we might have one day 3 slightly different dsl-s for different unwrapping/matching scenarios.
I don't see a way to prevent that tho, maybe even if it happens, they will converge eventually

@Araq
Copy link
Member

Araq commented Oct 9, 2018

The splitting of options was a request from Araq, I've got no issue reverting it if he's changed his mind.

I haven't, I still like the split.

@PMunch
Copy link
Contributor Author

PMunch commented Oct 9, 2018

I agree, the split is nice.

@krux02
Copy link
Contributor

krux02 commented Nov 8, 2018

First of all I would like to say, I like the split, but that is already part of the other PR. This is not about that. I don't like it, because it is too complicated. An option is nothing more that a value that might be there or that might be not there. You create an entire language about option expressions. This language probably has it's own quirks and is hard to maintain. I am sorry for your work that you put into this, but as the maintainer I have to reject this.

@krux02 krux02 closed this Nov 8, 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.

7 participants