Skip to content

Conversation

PMunch
Copy link
Contributor

@PMunch PMunch commented Jul 17, 2018

This creates a temporary symbol for the result of the maybe, so a procedure with side effects can be passed in as the first argument. And replaces the replaceIdents logic with simply creating a variable in the scope that takes the value of the maybe making it slightly more robust. It also fixes the slight issue where symbols would not work as the just and nothing keyword, something which apparently get's passed in sometimes.

@superfunc
Copy link
Owner

I'll take a look at this over the weekend, thanks @PMunch !

@superfunc
Copy link
Owner

One note, looks like its failing the basic.nim test. Running build_verify to see this in action. Let me know what you find.

@PMunch
Copy link
Contributor Author

PMunch commented Jul 20, 2018

So sorry, was busy doing this so completely forgot about this. The issue in basic.nim seems to be intentional, to avoid running procedures passed to maybe multiple times it stores the result in a temporary variable. I guess it technically could only do that if the input option was not a symbol though. But is assigning to the value of a maybe like that something you actually want to support? The same could be achieved with a = maybe.just(15).

@superfunc
Copy link
Owner

Hmm, I'm not sure, let me think it over for a day or so and I'll let you know.

@superfunc
Copy link
Owner

Given that fmap and the just constructor exist, I'm inclined to agree with you. If you update the PR to get basic passing I'll merge it in. Thanks!

@superfunc
Copy link
Owner

ping @PMunch

@PMunch
Copy link
Contributor Author

PMunch commented Aug 18, 2018

Again I'm sorry, I've focused on trying to get this into the official options module in Nim. But I fixed the test now :)

@superfunc
Copy link
Owner

No worries at all, I had forgotten about this too :). Very cool that you're looking to move the idea into the standard library, let me know how it goes if you can.

@superfunc superfunc merged commit 2ade024 into superfunc:master Aug 19, 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.

2 participants