Re: [PATCH] Add srfi-171 to guile

["Ludovic Courtès" <ludo@gnu.org>]

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’.