Thanks! I will address all those. The tests are written and passing, so I will remove the redundant TODOs.
-- Linus Björnstam On Sun, 8 Mar 2020, at 15:40, Ludovic Courtès wrote: > Hi Linus, > > Linus Björnstam <linus.bjorns...@veryfast.biz> skribis: > > > From c382d7808a8d41cd4e9ef8a17b7ba9553835499b Mon Sep 17 00:00:00 2001 > > From: =?UTF-8?q?Linus=20Bj=C3=B6rnstam?= <linus.bjorns...@fastmail.se> > > Date: Thu, 16 Jan 2020 20:31:45 +0100 > > Subject: [PATCH] Add SRFI-171 (transducers) to guile. > > > > The two guile-specific additions are powerful transducers which can be > > used to generalize transducers like tsegment. They are hard to get > > right, but powerful and useful enough to warrant inclusion. > > > > * doc/ref/srfi-modules.texi: added srfi-171 section > > * module/Makefile.am (SOURCES): > > * module/srfi/srfi-171.scm: > > * module/srfi/srfi-171/meta.scm: Add SRFI-171 > > * module/srfi/srfi-171/gnu.scm: Add 2 guile-specific extensions. > > * test-suite/Makefile.am (SCM_TESTS): > > * test-suite/tests/srfi-171.test: Add tests. > > I think the patch is almost ready for inclusion, thanks for taking the > time to address Andy’s comments! > > I have additional stylistic comments, and then I think we’re ready to go: > > > +Transducers are oblivious to what kind of process they are used in, and > > +are composable without building intermediate collections. This means we > > +can create a transducer that squares all even numbers: @code{(compose > > +(tfilter odd?) (tmap (lambda (x) (* x x))))} and reuse it with lists, > > For readability, it’s probably best to use @example rather than @code > for the example above (for an example larger than a couple of words in > general.) > > > +The central part of transducers are 3-arity reducing functions. > > In general, RnRS and Guile use the term “procedure” rather than > “functions”. Not a big deal, but bonus points if you can adjust the > documentation accordingly. :-) > > > +@itemize > > +@item > > +(): Produce an identity > > s/an/the/ ? > > > +@subheading The concept of transducers > > + > > +A transducer is a one-arity function that takes a reducer and produces a > > +reducing function that behaves as follows: > > + > > +@itemize > > +@item > > +(): calls reducer with no arguments (producing its identity) > > It’s not clear from this where you’d write () (there’s a missing @code > here, right?), which is not a valid Scheme expression in itself. > Perhaps an extra bit of introduction is needed above for clarity? > > Also, this is under the “Concept” heading, but it looks more like the > API, no? > > > +@item > > +(result-so-far): Maybe transform the result-so-far and call reducer with > > it. > > + > > +@item > > +(result-so-far input) Maybe do something to input and maybe call the > > +reducer with result-so-far and the maybe-transformed input. > > Missing @code ornaments here. > > > +a simple example is as following: @code{ (list-transduce (tfilter odd?) > ^ > Capital. > > @example for the example. > > > ++ '(1 2 3 4 5))}. This first returns a transducer filtering all odd > > +elements, then it runs @code{+} without arguments to retrieve its > > +identity. It then starts the transduction by passing @code{+} to the > > Please make sure to add two spaces after end-of-sentence periods > throughout the document. > > > + Even though transducers appear to be somewhat of a generalisation of > > + map and friends, this is not really true. Since transducers don't know > > s/map/@code{map}/ > > > +@deffn {Scheme Procedure} list-transduce xform f lst > > +@deffnx {Scheme Procedure} list-transduce xform f identity lst > > + Initializes the transducer @code{xform} by passing the reducer @code{f} > > + to it. If no identity is provided, @code{f} is run without arguments to > > + return the reducer identity. It then reduces over @code{lst} using the > > + identity as the seed. > > Please remove the extra leading space. Also, use @var, and use > imperative tense, like so: > > @deffn {Scheme Procedure} list-transduce @var{xform} @var{f} @var{lst} > Initialize the transducer @var{xform} by passing the reduce @var{f} to > it. … > @end deffn > > > +If one of the transducers finishes early (such as @code{ttake} or > > +@code{tdrop}), it communicates this by returning a reduced value, which > > +in the sample implementation is just a value wrapped in a SRFI 9 record > > +type named "reduced". If such a value is returned by the transducer, > > For proper typesetting, write ``reduced'' instead of "reduced". > > s/SRFI /SRFI-/ > > > +Same as @code{list-transduce}, but for vectors, strings, u8-bytevectors > > +and srfi-158-styled generators respectively. > > + > > +@end deffn > > Please remove the newline before @end deffn. > > > +@node SRFI-171 Helpers > > +@subsubsection Helper functions for writing transducers > > +@cindex transducers helpers > > + > > +These functions are in the @code{(srfi 171 meta)} module and are only > ^ > (srfi srfi-171 meta) > > > +@deffn {Scheme Procedure} reduced value > > + > > +Wraps a value in a @code{<reduced>} container, signalling that the > > +reduction should stop. > > +@end deffn > > Please remove extra line before @deffn (throughout the document). > > > +(define-module (srfi srfi-171) > > + #:declarative? #t > > Is it necessary? If so, could you add a comment so our future selves > know whether this is still necessary? :-) > > > +(define reverse-rcons > > + (case-lambda > > + "A transducer-friendly consing reducer with '() as identity. > > +The resulting list is in reverse order." > > In general, the style for docstrings is to use imperative tense, like: > > Return a consing reducer with the empty list as its identity. > > (Throughout the file.) > > Could you also add docstrings to the exported procedures that lack one? > (Docstrings can be short of course, no need to be as precise as in the > manual.) > > > +++ b/test-suite/tests/srfi-171.test > > @@ -0,0 +1,195 @@ > > +;; TODO: test all transducers that take an equality predicate > > +;; TODO: test treplace with all kinds of hash tables > > Should we wait for these tests before merging? Tested code is always > better than untested code. :-) > > Also, please add a Guile copyright header. > > > + (pass-if "tfilter+tmap" (equal? > > + '(1 3 5) > > + (list-transduce (compose (tfilter even?) (tmap > > add1)) rcons numeric-list))) > > Please wrap lines to 80 chars. > > Could you send an updated patch? > > Thank you! > > Ludo’. >