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

Make --> bind IT for use anywhere in FORMS, and add -as->. #221

Merged
merged 2 commits into from Jun 13, 2017

Conversation

Projects
None yet
2 participants
@zck
Contributor

zck commented May 25, 2017

This code fixes #218, in a way that I think is fairly straightforward, and not error-prone. I've added a example to show the problem that was fixed.

It keeps the feature that if a FORM is a symbol, the code will treat that symbol as a function, and pass it the single arg it. I keep this only because it's a legacy feature; it's not how Clojure operates, and was surprising to me. But, as mentioned, I kept it.

This also fixes a multiple-evaluation bug; if it appears multiple times in a FORM, the value will be evaluated multiple times. The following code prompts for input twice with the old -->, but only once with this version.

(--> (progn (read-from-minibuffer "How many prompts? ") 3) (+ it it))

As a helper method, this adds a new method -as->, which is a version of --> that takes the variable explicitly, as opposed to --> only using it. I think the standard dash.el nomenclature would be to name the anaphoric version with an extra dash, but the anaphoric version is -->, and the version with one fewer dash (->) is already used. Please let me know if there's a better name for this new function. The existing name is a riff on Clojure's, but beginning with a dash to fit in with names here.

I'm happy to make changes, whether to the name, or anything else here.

@zck

This comment has been minimized.

Show comment
Hide comment
@zck

zck May 25, 2017

Contributor

Also to mention: I think I have the declarestatements right, but am not sure. I was able to debug into the macros, but I have very little experience using edebug, so I could very well have done it incorrectly and not noticed, because I'm not sure how edebug is supposed to work.

Contributor

zck commented May 25, 2017

Also to mention: I think I have the declarestatements right, but am not sure. I was able to debug into the macros, but I have very little experience using edebug, so I could very well have done it incorrectly and not noticed, because I'm not sure how edebug is supposed to work.

@Fuco1 Fuco1 self-requested a review May 25, 2017

@zck

This comment has been minimized.

Show comment
Hide comment
@zck

zck May 29, 2017

Contributor

I'm not sure why, but the Travis CI build has finished successfully, but hasn't reported back here.

Contributor

zck commented May 29, 2017

I'm not sure why, but the Travis CI build has finished successfully, but hasn't reported back here.

@Fuco1

This comment has been minimized.

Show comment
Hide comment
@Fuco1

Fuco1 May 29, 2017

Collaborator

I marked this for a review. I played a bit around and couldn't find a counter example where it would break but I'm still kind of afraid that some of the anaphora macros will expand in a stupid way and things will get ugly. Going over each one is out of the question I guess, that would be just too much work.

Or maybe we can just try it and see... I hope nobody's powering spacecraft with Emacs :P

Collaborator

Fuco1 commented May 29, 2017

I marked this for a review. I played a bit around and couldn't find a counter example where it would break but I'm still kind of afraid that some of the anaphora macros will expand in a stupid way and things will get ugly. Going over each one is out of the question I guess, that would be just too much work.

Or maybe we can just try it and see... I hope nobody's powering spacecraft with Emacs :P

@zck

This comment has been minimized.

Show comment
Hide comment
@zck

zck May 31, 2017

Contributor

That's fair. I might go through and add some more examples using more complicated anaphoric macros.

Also I tried to find a space game or something in Emacs to make a joke about, but couldn't find one. I'm pretty disappointed, to be honest.

Contributor

zck commented May 31, 2017

That's fair. I might go through and add some more examples using more complicated anaphoric macros.

Also I tried to find a space game or something in Emacs to make a joke about, but couldn't find one. I'm pretty disappointed, to be honest.

@zck

This comment has been minimized.

Show comment
Hide comment
@zck

zck Jun 11, 2017

Contributor

Here are some examples mixing --> with other anaphoric operators. They're not good examples of using the dash library, so I don't think we should add them to the examples, but they're useful for seeing if the macros have problems with multiple its. Although if you prefer to add them as tests, let me know and I'll do that.

ELISP> (--> '(1 2 3 4) (--map (1+ it) it))
(2 3 4 5)
ELISP> (--map (--> it (1+ it)) '(1 2 3 4))
(2 3 4 5)

ELISP> (--filter (--> it (equal 0 (mod it 2))) '(1 2 3 4))
(2 4)
ELISP> (--> '(1 2 3 4) (--filter (equal 0 (mod it 2)) it))
(2 4)

ELISP> (--annotate (--> it (< 1 it)) '(0 1 2 3))
((nil . 0)
 (nil . 1)
 (t . 2)
 (t . 3))

ELISP> (--> '(0 1 2 3) (--annotate (< 1 it) it))
((nil . 0)
 (nil . 1)
 (t . 2)
 (t . 3))
Contributor

zck commented Jun 11, 2017

Here are some examples mixing --> with other anaphoric operators. They're not good examples of using the dash library, so I don't think we should add them to the examples, but they're useful for seeing if the macros have problems with multiple its. Although if you prefer to add them as tests, let me know and I'll do that.

ELISP> (--> '(1 2 3 4) (--map (1+ it) it))
(2 3 4 5)
ELISP> (--map (--> it (1+ it)) '(1 2 3 4))
(2 3 4 5)

ELISP> (--filter (--> it (equal 0 (mod it 2))) '(1 2 3 4))
(2 4)
ELISP> (--> '(1 2 3 4) (--filter (equal 0 (mod it 2)) it))
(2 4)

ELISP> (--annotate (--> it (< 1 it)) '(0 1 2 3))
((nil . 0)
 (nil . 1)
 (t . 2)
 (t . 3))

ELISP> (--> '(0 1 2 3) (--annotate (< 1 it) it))
((nil . 0)
 (nil . 1)
 (t . 2)
 (t . 3))
@Fuco1

This comment has been minimized.

Show comment
Hide comment
@Fuco1

Fuco1 Jun 12, 2017

Collaborator

Yes please, add them as tests and then we merge.

Only the first three examples are picked for the documentation so if we don't want to show these add three "good ones" and place these after that.

Collaborator

Fuco1 commented Jun 12, 2017

Yes please, add them as tests and then we merge.

Only the first three examples are picked for the documentation so if we don't want to show these add three "good ones" and place these after that.

@zck

This comment has been minimized.

Show comment
Hide comment
@zck

zck Jun 13, 2017

Contributor

I added the tests.

I added them as a separate commit, but I can squash them if you prefer.

Contributor

zck commented Jun 13, 2017

I added the tests.

I added them as a separate commit, but I can squash them if you prefer.

@Fuco1

This comment has been minimized.

Show comment
Hide comment
@Fuco1

Fuco1 Jun 13, 2017

Collaborator

No problem! Thanks for your contribution!

Collaborator

Fuco1 commented Jun 13, 2017

No problem! Thanks for your contribution!

@Fuco1 Fuco1 merged commit 1e14307 into magnars:master Jun 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment