-
Notifications
You must be signed in to change notification settings - Fork 137
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 -to-head
and -shuffle
#404
base: master
Are you sure you want to change the base?
Conversation
* dash.el (-to-head): New function. * dev/examples.el (-to-head): New example. * README.md: * dash.texi: Regenerate docs.
* dash.el (-shuffle): New function. * dev/examples.el (-shuffle): New example. * README.md: * dash.texi: Regenerate docs.
I found that using the So I plan to change the API of
|
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.
Thanks for working on this!
So I plan to change the API of
-shuffle
to(fn LIST &optional RNG)
If we really can't do without such an optional argument, then I at least don't think we should expose it as part of the advertised calling convention, at least not until it's really proven to be an essential part of the API.
Neither Clojure nor Racket, for example, expose such an argument in their standard shuffle routines.
So I'm not yet convinced that we need it.
In our test, we can use xorshift based RNG to create a deterministic random number sequence
For better or worse, dev/examples.el
is a mix of both user-facing examples and developer-facing unit/regression tests.
I believe that the user-facing examples should be as simple and transparent as possible, and need not be comprehensive. Taking a custom RNG as argument for the sake of our test suite seems like the tail wagging the dog. Unless someone brings forth a compelling argument for such an API that does not arise from testing needs, I think we can do without such an argument for now.
In our more comprehensive developer-oriented implementation tests, OTOH, we can do whatever we like. We can have internal ert-deftest
s at the end of dev/examples.el
, -shuffle
can call an internal function that takes an RNG argument for better testability, etc. But the public API should not care about these details.
WDYT?
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.
README.md
is autogenerated from function docstrings, the top three examples of each defexamples
in dev/examples.el
, and readme-template.md
. So the docstrings and first three examples should be README.md
-quality, and this file should not be edited by hand.
@@ -3238,6 +3238,30 @@ if the first element should sort before the second." | |||
(declare (debug (def-form form))) | |||
`(-sort (lambda (it other) (ignore it other) ,form) ,list)) | |||
|
|||
(defun -to-head (n list) | |||
"Return a new list that move the element at Nth to the head of old LIST." |
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.
Grammatical nits:
move -> moves
element at Nth -> Nth element
head of old LIST -> head of the old LIST
But I think we can make this even shorter, to fit even within the default 65 column suggestion:
"Return a copy of LIST with its Nth item moved to the front."
WDYT?
(if (> n (1- (length list))) | ||
(error "Index %d out of the range of list %S" n list)) |
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 it better to signal here, or return the original list unchanged? There is a lot of precedent for the latter behaviour, e.g. nth
, -remove-at
, etc.
BTW, there's a special args-out-of-range
error for out-of-range sequence access.
(let* ((head (-take n list)) | ||
(rest (-drop n list)) |
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.
What about using -split-at
here?
The returned list is shuffled by using Fisher-Yates' Algorithm. See | ||
https://en.wikipedia.org/wiki/Fisher-Yates_shuffle for more details." |
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 think the implementation is better described in a comment than the docstring.
|
||
The returned list is shuffled by using Fisher-Yates' Algorithm. See | ||
https://en.wikipedia.org/wiki/Fisher-Yates_shuffle for more details." | ||
(declare (pure t) (side-effect-free 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.
Shuffling returns a different result each time, so it is not pure, and it is side-effect-free only if we remove the rng
argument like I suggest.
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.
Ditto re: autogenerating this file.
|
||
```el | ||
(-to-head 3 '(1 2 3 4 5)) ;; => (4 1 2 3 5) | ||
(-to-head 5 '(1 2 3 4 5)) ;; peculiar error |
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.
BTW, peculiar error
means there is a bug in our code, so it should not happen or appear in our docs.
(-shuffle '(1 2 3 4 5 6 7) (make-xorshift32-rng #xcafe)) => '(7 6 1 2 3 5 4) | ||
(-shuffle '(1 2 3 4 5 6 7) (make-xorshift32-rng #xbeef)) => '(4 3 2 5 6 7 1) | ||
(let ((l '(1 2 3 4 5 6 7))) | ||
(list (-shuffle '(1 2 3 4 5 6 7) (make-xorshift32-rng #xdead)) l)) => '((3 4 6 5 1 2 7) (1 2 3 4 5 6 7)) |
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.
make-xorshift32-rng
is completely opaque to the user, so if we really can't do without it, it should at least not appear in our top examples/docs. Its name should also follow standard Elisp conventions with a file prefix and internal double hyphen --
.
Continue #291
Close #283 & #285
Cc: @debanjum