Add -to-head and -shuffle #404
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
|
basil-conto
left a comment
basil-conto
left a comment
There was a problem hiding this comment.
Thanks for working on this!
So I plan to change the API of
-shuffleto(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-deftests 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.
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.
| `(-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.
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.
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.
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.
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.
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.
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.
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.
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 --.
basil-conto
added
the
enhancement
2 participants
Continue #291
Close #283 & #285
Cc: @debanjum