-
Notifications
You must be signed in to change notification settings - Fork 105
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
#236 - Generalize continuous-subseqs #294
base: master
Are you sure you want to change the base?
Conversation
Adding a currently failing test to spark a discussion on how best to handle this scenario (i.e. creating a predicate function that correctly handles seeing only the beginning, but not ending, of a subsequence). |
test/com/rpl/specter/core_test.cljc
Outdated
@@ -960,7 +960,26 @@ | |||
(is (= [[] [2] [4 6]] | |||
(select | |||
[(s/continuous-subseqs number?) (s/filterer even?)] | |||
[1 "a" "b" 2 3 "c" 4 5 6 "d" "e" "f"])))) | |||
[1 "a" "b" 2 3 "c" 4 5 6 "d" "e" "f"]))) | |||
(defn- make-bounds-pred-fn [start end] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple comments here:
- The defn should be a top-level form
- I'm struggling to understand this function. Shouldn't it be returning true or false? Why does it return
start
andend
in some cases. Isprev
the previous element or true/false for the run of this function on the previous element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a little dicey. Basically, it's trying to have the "previous" value do double duty: both signaling that the item should be included in a selected subsequence (i.e. true
), but also carrying the previous boundary keyword (if applicable), without indicating that keyword should be included in a subsequence. So, more or less the same concept as outlined in the issue #236 description.
I tried changing the predicate around so that it returns a vector of both of these separately, but it seems to get a bit gnarly trying to handle that in the transducer:
(let [index-results (into [] (subseq-pred-fn-transducer p) aseq)]
(map last (filter (comp true? first) index-results)))
I suppose the caller could be asked to supply an additional function to subseq-pred-fn
that controls how the truthy piece is pulled out of the results for the purpose of building the actual subsequence indeces?
function (taking the previous and current item), to preserve backward compatibility (still allowing predicate functions that only take the current item). This macro also takes in a get-truthy-fn as its first argument, which is a function that marks whether that item in the sequence should be included in a subsequence. This is necessary because the predicate function can now be of any arbitrary form, so we cannot make any assumption about how the user intends for that result to be interpreted as a "filter". Adding SubseqsDynamicPredFn, which works the same as SrangeEndFn, to support backward compatibility Adding wrapper to take a predicate on [prev current] and turn it into a predicate also taking the current index as the first param Creating transducer to combine this with the user-supplied predicate function Adding tests for select and transform TODO: figure out how to make predicate function handle an open-ended subsequence (ex: end marker not yet seen)
I took another crack at this, this time adding an additional parameter to the macro so the user can specify their "truthy function". |
Adding new subseq-pred-fn macro to create the new form of predicate function (taking the previous and current item), to preserve backward compatibility (still allowing predicate functions that only take the current item)
Adding SubseqsDynamicPredFn, which works the same as SrangeEndFn, to support backward compatibility
Adding wrapper to take a predicate on [prev current] and turn it into a predicate also taking the current index as the first param
Creating transducer to combine this with the user-supplied predicate function
Adding tests for select and transform
WORK IN PROGRESS
TODO: figure out how to make predicate function handle an open-ended subsequence (ex: end marker not yet seen)