Add Navigators for Sorted Sequences #317
Conversation
| Value collection (e.g. collect, collect-one) may not be used in the keypath." | ||
| ([keypath] (sorted-by keypath compare)) | ||
| ([keypath comparator] | ||
| (late-bound-nav [late (late-path keypath) |
There was a problem hiding this comment.
Either the naming or implementation here is off. As named, this isn't accepting general paths but only a key. In this case, it's wrong to use late-path and it will be more straightforward/faster to use get rather than compiled-select. Additionally, compiled-select returns a sequence of results which I don't think you want to be comparing against.
An alternative is to have the first arg be an "extractor path", and use compiled-select-one! to extract the key. This would allow for usage where the desired comparator key is nested or transformed, like (sorted-by [:a :b (view -)])
There was a problem hiding this comment.
Yeah, my intention was to have it be a generic full path, what you're calling an extractor path. I thought keypath would be an appropriate term here since in clojure.core/sort-by the argument is called the keyfn. I'll change this to use compiled-select-one!.
There was a problem hiding this comment.
I've updated this to use compiled-select-one!. Would you recommend I also rename the keypath, or is this enough?
src/clj/com/rpl/specter/impl.cljc
Outdated
| indices (map first sorted) | ||
| result (next-fn (map second sorted)) | ||
| unsorted (sort-by first compare (map vector (concat indices (repeat ##Inf)) result))] | ||
| (into (empty structure) |
There was a problem hiding this comment.
Does this work with lists? The issue with (into '() ...) is it prepends for each element, oftentimes reversing the output order of what you expect. This needs a test at least.
There was a problem hiding this comment.
That's a very good point, I'll add a test for this.
There was a problem hiding this comment.
I've updated this, it has a test and works with lists as well as vectors.
|
Are there any other changes you'd like me to do @nathanmarz before it would be ready for a merge? |
|
Oh I was looking for this for a use case I have got, any recommended alternative? |
This PR adds support for three new navigators which all navigate to a sorted version of the current sequence.
SORTED, a parameter-less navigator which navigates to a transformable view of the current sequence as if it were run throughsortsorted, a single-parameter navigator which is the same asSORTEDbut takes a comparatorsorted-by, a dynamic navigator which takes a keypath for the sort key and an optional comparatorBefore this PR can be merged, the following steps must be completed by the author:
This PR implements what was discussed in #316.