Re: [PATCH] Add srfi-171 to guile

["Linus Björnstam" <linus.bjornstam@veryfast.biz>]

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.bjornstam@veryfast.biz> skribis:
> 
> > From c382d7808a8d41cd4e9ef8a17b7ba9553835499b Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Linus=20Bj=C3=B6rnstam?= <linus.bjornstam@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’.
>