add: -interleave-any #401
add: -interleave-any #401
Conversation
|
Doc string of |
|
This should be named |
|
You're right, that's much better. Done. |
basil-conto
force-pushed
the
interleave-any
branch
from
3d6c30c
to
700ee3d
Compare
There was a problem hiding this comment.
Thanks, this is a welcome addition!
See my comments below for some minor nitpicks mostly related to Emacs/Dash conventions.
My only real request is that you please add some examples/tests for -interleave-all in dev/examples.el. The first three examples listed there will automatically be added to README.md and dash.texi. There is no problem if you are unable or unwilling to regenerate these docs; someone else will do that before/after merging.
BTW, I think this PR is small enough to be exempt from copyright assignment (CA) to the FSF; see (info "(emacs) Copyright Assignment"). However, if it's possible you'll contribute again in the future, I would recommend getting started on the CA process now, so that you're ready to go when the time comes. If you are willing to do this and have not already done so, please fill out and email the following form, and you'll be instructed on how to proceed by the copyright clerk: https://git.sv.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-assign.future
dash.el
Outdated
| "Return a new list of the first item in each of `lists', then the | ||
| second etc. Continue interleaving all elements of all lists by | ||
| skipping the non-existing elements of short lists." |
There was a problem hiding this comment.
Nit: Please follow the Emacs docstring conventions outlined in (info "(elisp) Documentation Tips") and implied by this project's dir-locals-file, specifically:
- The first sentence (summary) of the docstring should fit on a single line.
- Argument names (metavariables) should be formatted as
LISTSrather than as`lists'. - Sentences should end with a full stop followed by two spaces.
There was a problem hiding this comment.
This should be fixed in -interleave (and other docstrings) as well then.
There was a problem hiding this comment.
I adjusted it to "Return a new list containing an element of each of `LISTS' in turn, including all elements. ".
There was a problem hiding this comment.
Wait, that doesn't even fit into the insane 70 fill-column. I have no idea how to describe this in a good way that fits into 70 chars. :(
There was a problem hiding this comment.
This should be fixed in
-interleave(and other docstrings) as well then.
Of course, and you're more than welcome to include a fix for that as well! ;)
Dash is a large library that historically didn't follow all Emacs conventions, so fixing everything at once is too big a task in practice. Personally whenever I get the time I try to leave whichever parts I touch in a better state than I found them, and I try not to introduce new non-compliant code, even if it's closely related to existing non-compliant code. Eventually, everything will be consistent, or at least that's the hope. :)
Wait, that doesn't even fit into the insane 70 fill-column. I have no idea how to describe this in a good way that fits into 70 chars. :(
The only thing that has to be less than 70-80 characters is the first line, the summary. I know coming up with a useful summary is hard, but once that's achieved the rest of the docstring can elaborate on what the summary is trying to say.
basil-conto
added
the
enhancement
|
Done, incorporated the comments. |
|
Any input on this? |
dash.el
Outdated
| (declare (pure t) (side-effect-free t)) | ||
| (when lists | ||
| (let (result) | ||
| (while (--some (consp it) lists) |
There was a problem hiding this comment.
Nit: this is the same as (--some it lists), right?
dash.el
Outdated
| @@ -1594,6 +1594,18 @@ elements of LIST. Keys are compared by `equal'." | |||
| (setq lists (-map 'cdr lists))) | |||
| (nreverse result)))) | |||
|
|
|||
| (defun -interleave-all (&rest lists) | |||
| "Return a new list containing an element of each of `LISTS' in turn, including all elements. " | |||
There was a problem hiding this comment.
IMO the first version of this docstring was a much clearer description. :) The only problem with it was that the first sentence wasn't a summary that fit entirely on a single line. Apart from clarity, the current version has three technical issues:
- As you found out, it is longer than 80 characters, which emits an error during byte-compilation in Emacs 28+ and thus CI fails.
- Metavariables are written unquoted, i.e.
`LISTS'->LISTS. - There is spurious trailing whitespace after the full stop.
Suggest writing something like the following instead:
"Zip all elements from each of LISTS into a new flat list.
That is, return a list comprising the first item from each of
LISTS in turn, followed by their second items, etc.
Continue interleaving until all elements from all LISTS are
included, skipping non-existing elements from shorter LISTS.
This is like `-interleave', but the interleaving continues until
all input elements are consumed, instead of stopping after one of
LISTS becomes empty."| (-interleave '(1 2) '("a" "b")) => '(1 "a" 2 "b") | ||
| (-interleave '(1 2) '("a" "b") '("A" "B")) => '(1 "a" "A" 2 "b" "B") |
There was a problem hiding this comment.
-interleave -> -interleave-all
An
-interleavethat interleaves all elements of all lists.