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

FR: -sort-keyed #156

Open
doublep opened this issue Aug 26, 2015 · 8 comments · May be fixed by #166
Open

FR: -sort-keyed #156

doublep opened this issue Aug 26, 2015 · 8 comments · May be fixed by #166
Labels
enhancement Suggestion to improve or extend existing behavior
Milestone

Comments

@doublep
Copy link
Contributor

doublep commented Aug 26, 2015

I have already written the function, but then noticed in the docs that it can easily be composed from existing stuff:

(-sort-keyed key-computer comparator list) <==> (-sort (-on comparator key-computer) list)

Therefore I didn't finish my work and decided to ask if it is wanted first.

I still think that the function is useful:

  • it calls key-comparator exactly N times, while existing function calls it O (N log N) times, which makes a significant difference if N is large and key-computer is non-trivial;
  • its invocation is arguably easier to understand.

If this function is wanted, I can finish what I started with documentation and tests.

@Fuco1
Copy link
Collaborator

Fuco1 commented Aug 26, 2015

So basically you map, sort with remembering the order and then unmap?

@doublep
Copy link
Contributor Author

doublep commented Aug 26, 2015

I build list of conses of keys and values, sort that, and than map to discard keys. Here it is:

(defun -sort-keyed (key-computer comparator list)
  "Sort LIST by given keys, comparing them using COMPARATOR.
First KEY-COMPUTER is used to compute key value for each list
item.  Then resulting keys are sorted (stably) according to
COMPARATOR.  Finally, a copy of the original LIST is built and
rearranged in the same way.

LIST is not modified by side effects.  KEY-COMPARATOR is called
once for each element of LIST, i.e. with one argument.
COMPARATOR is called with two keys, and should return non-nil if
the first one (and thus its associated LIST element) should sort
before the second."
  (if (cdr list)
      (let (keyed-list)
        (--each list
          (!cons (cons (funcall key-computer it) it) keyed-list))
        ;; Reversing before sorting seems unnecessary, but it ensures
        ;; stability by restoring original order.  We don't use
        ;; '-call' because there's no need to copy argument.
        (--map (cdr it)
               (sort (nreverse keyed-list)
                     (lambda (a b) (funcall comparator (car a) (car b))))))
    (when list
      (list (car list)))))

@Fuco1
Copy link
Collaborator

Fuco1 commented Aug 26, 2015

Yea, this already exists in dash. The problem is that it doesn't work well without lexical scope (the lambda there can break quite easily). See -grade-up or -grade-down

@doublep
Copy link
Contributor Author

doublep commented Aug 26, 2015

No, the function I proposed is the same as (-sort (-on comparator key-computer) list), only more efficient in some cases. It doesn't return list of indices, but sorted copy of original list.

What is the problem with lambdas and non-lexical binding? That you can accidentally shadow a variable user depends on?

@Fuco1
Copy link
Collaborator

Fuco1 commented Aug 26, 2015

Consider for example

(let ((comparator (lambda (a b) (let (comparator) (funcall comparator a b)))))
  (sort '(1 2 3 4) comparator))

@Fuco1
Copy link
Collaborator

Fuco1 commented Aug 26, 2015

Well, that's a bit silly example... I'll try to come up with something better :D I'm not sure now it showcases what I wanted.

@Fuco1
Copy link
Collaborator

Fuco1 commented Oct 4, 2015

Can you open a PR with your implementation so we can review it?

@doublep doublep linked a pull request Mar 31, 2016 that will close this issue
@Fuco1
Copy link
Collaborator

Fuco1 commented May 1, 2016

Followed in the PR.

@Fuco1 Fuco1 closed this as completed May 1, 2016
@basil-conto basil-conto reopened this Feb 15, 2021
@basil-conto basil-conto linked a pull request Feb 15, 2021 that will close this issue
@basil-conto basil-conto added the enhancement Suggestion to improve or extend existing behavior label Feb 15, 2021
@basil-conto basil-conto added this to the 2.19.0 milestone Feb 15, 2021
@basil-conto basil-conto added this to To Do in Release 2.19.0 Mar 9, 2021
@basil-conto basil-conto modified the milestones: 2.19.0, 2.20.0 Jun 29, 2021
@basil-conto basil-conto removed this from To Do in Release 2.19.0 Jun 29, 2021
@basil-conto basil-conto added this to To Do in Release 2.21.0 Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Suggestion to improve or extend existing behavior
Projects
Development

Successfully merging a pull request may close this issue.

3 participants