Skip to content
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 -separate-multi and --separate-multi. #299

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add -separate-multi and --separate-multi. #299

wants to merge 3 commits into from

Conversation

Luis-Henriquez-Perez
Copy link

Concerning issue #230.

Copy link
Collaborator

@basil-conto basil-conto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some docstring nitpicks from me.

dash.el Outdated
(defun -separate-multi (preds list)
"Return a list of ((-filter PRED1 LIST) (-filter PRED2 LIST) ... REST), in one
pass through the list. PREDS is a list of functions and REST are elements of
LIST that are unmatched by any PREDS."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the Emacs docstring conventions as documented under (elisp) Documentation Tips. In particular: the first line should be a brief but complete sentence; the subsequent body of the docstring should not be indented; and each full stop at the end of a sentence should be followed by two spaces.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, I just added more commits to fix this. Do you think I should take out the ((-filter PRED1 LIST)...) part? Is it confusing because REST is not a parameter? If I leave that in I can't fit in one pass through the list on the first line. As it is I separated it into it's own line.

dash.el Outdated
(--map (nreverse (cdr it)) preds))

(defmacro --separate-multi (forms list)
"Anaphoric form of -separate-multi."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please quote symbol references as `-separate-multi' so that the help system can hyperlink them, as per the aforementioned conventions.

@alphapapa
Copy link

As I mentioned on #230 (comment), please consider making the return value a cons of two lists: lists of matching items in the CAR, and non-matching items in the CDR. This would make accessing the results and determining whether there are matching and non-matching items simpler and faster.

@Fuco1
Copy link
Collaborator

Fuco1 commented May 4, 2019

Why is there a merge commit?

@Fuco1 Fuco1 self-requested a review May 4, 2019 12:26
Copy link
Collaborator

@Fuco1 Fuco1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a review to block merging until all the comments are solved

@Fuco1
Copy link
Collaborator

Fuco1 commented May 4, 2019

@basil-conto can you start a review with a change request? I'm not sure if people outside of the repo can do that. We could probably add you to collaborators anyway.

@Luis-Henriquez-Perez
Copy link
Author

Why is there a merge commit?

That was me fixing the comment problems @basil-conto mentioned. This is my first pull request so let me know if I messed something up trying to fix things.

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.

4 participants