Hello,

thanks for the review!  My reactions are below.

On 2023-09-20 00:57:33 +0200, Maxime Devos wrote:
> 
> 
> Op 06-09-2023 om 16:06 schreef Tomas Volf:
> > Extend the documentation for (ice-9 match) module with explanation of
> > some of the patterns and also provide more examples for them.  That
> > should make it more useful for people trying to use the module for the
> > first time just based on the documentation.
> >
> > * doc/ref/match.texi (Pattern Matching): Explain some patterns and
> > provide more examples
> > ---
> > When I tried to use (ice-9 match) for the first time, I was not able to,
> > except the most basic usage.  The documentation was not very helful, so
> > I went to the IRC.  I got help, but I was also asked to try to
> > contribute to the docs with examples that would have helped me in the
> > first place.
> >
> > I think even more additions would be useful, but I still do not
> > understand the pattern matching fully, so they are not a part of this
> > patch.  Examples would be: How and when I should use quasi-patterns?
> > What does `ooo' stand for?  Does `box' refer to SRFI-111 or something
> > else?
> Note: there is a previous patch for improving (ice-9 match) documentation
> named ‘[PATCH v3] docs/match: pattern matcher example makeover’, though IIRC
> it needed some changes.  Maybe you can adapt some things from it.  As long
> as it doesn't descend in fake gatekeeping accusations again, it would be
> nice if it wasn't for nothing.

I managed to find it, and there even seems to be a v4.  I sadly do not have the
time to work through it right now, and I do not want to make any promises
regarding if/when I will have it.  So for now I incorporated your feedback and
will sent it as v v2.

> 
> Smaller incremental improvements like this would also be nice, though.
> 
> >
> > I was following the HACKING file, so I added myself into the THANKS
> > file, I am not sure if I was supposed to for this relatively trivial
> > change.
> I think you aren't supposed to (whether it is trivial or not).  Instead, I
> think that others are supposed to add you to THANKS, otherwise you would be
> thanking yourself.  That is, however, a personal disagreement I have with
> HACKING.
> 
> I think that triviality or not is irrelevant; time spent on evaluating
> whether something is trivial or not is better spent on doing more
> contributors.  Besides, THANKS says ‘Contributors since the last release’,
> not ‘Contributors of non-trivial changes since the last release’.

Makes sense, removed.

> 
> >
> >   THANKS             |  1 +
> >   doc/ref/match.texi | 75 ++++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 76 insertions(+)
> >
> > diff --git a/THANKS b/THANKS
> > index aa4877e95..bb07cda83 100644
> > --- a/THANKS
> > +++ b/THANKS
> > @@ -38,6 +38,7 @@ Contributors since the last release:
> >                BT Templeton
> >             David Thompson
> >              Bake Timmons
> > +	  Tomas Volf
> >            Mark H Weaver
> >             Göran Weinholt
> >              Ralf Wildenhues
> > diff --git a/doc/ref/match.texi b/doc/ref/match.texi
> > index f5ea43118..b5a0147fa 100644
> > --- a/doc/ref/match.texi
> > +++ b/doc/ref/match.texi
> > @@ -181,6 +181,81 @@ The names @code{quote}, @code{quasiquote},
> @code{unquote},
> >   @code{or}, @code{not}, @code{set!}, @code{get!}, @code{...}, and
> >   @code{___} cannot be used as pattern variables.
> >
> > +@code{string}, @code{number}, and others refer to literal strings,
> > +numbers, and others.  Therefore pattern @code{string} binds the value to
> > +identifier @code{string}, pattern @code{"string"} matches if the value
> > +is @code{"string"}.
> It would do pause after ‘therefore’, not ‘before’.  Might just be a personal
> thing, though.  Also, something about joining two sentences with a comma
> instead of ‘and’ bothers me.  Also², I find it difficult to tell whether
> adding ‘the’ before ‘pattern’ would be better or worse. What do you think?

Maybe.  I am not that skilled with articles, so I defer to you here.  It at
least does not sound worse to me.

> 
> ‘Therefore, pattern @code{string} binds the value to
> 
> > Therefore the pattern @code{string} binds the value to
> > the identifier @code{string} and the pattern [...].
> 
> >  Example demonstrating this (by using very bad
> > +naming):
> Maybe ‘An example’?  But again, dunno.

Here as well.

> 
> > +
> > +@example
> > +(let ()
> > +  (match "foo"
> > +    (number number)))
> > +@result{} "foo"
> > +@end example
> > +
> > +The field operator (@code{(= field pat)}) has no relation to the fields
> > +of SRFI-9 records.  The @code{field} should be an expression evaluating
> > +to a procedure taking a single argument, and @code{pat} is matched
> > +against the return value.  Simple example:
> 
> s/SRFI-9 records/records
> 
> There's other methods for defining records to that it has no relation too!

Ah, good point.

> 
> > +@example
> > +(let ()
> > +  (match '(1 2)
> > +    ((= cadr x)
> > +     x)))
> > +@result{} 2
> > +@end example
> > +
> > +The record operator(@code{($ record-name pat_1 ... pat_n)}) can be used
> > +for matching SRFI-9 and SRFI-99 records.  Patterns are matched against
> > +the slots in order, not all have to be present, and there is no way to
> > +skip a slot.  Example demonstrating the usage:
> Probably also RNRS records, structs and (ice-9 records), though I haven't
> tested that.
> 
> Basically, all record types except perhaps GOOPS.
> 
> Though perhaps no slot order is guaranteed for RNRS and GOOPS, even if it
> happens to work currently?  Dunno.
> 
> I don't think it's _supposed_ to work with opaque RNRS records, but that's
> probably not yet implemented.

I decided to just leave out the type and went with "for matching records."
There seems to be many kinds, and I have no idea with which it does work.  Being
less precise (no type) is probably better than being wrong.

> 
> > +@example
> > +(define-record-type <foo>
> > +  (make-foo bar baz zab)
> > +  foo?
> > +  (bar foo-bar)
> > +  (baz foo-baz)
> > +  (zab foo-zab))
> > +
> > +(let ((obj (make-foo 1 '2 "three")))
> > +  (match obj
> > +    ;; Make sure obj is a <foo> instance, with bar slot being a number
> > +    ;; and zab slot being a string.  We do not care about baz slot,
> > +    ;; therefore we use _ to match anything.
> > +    (($ <foo> (? number?) _ (? string?))
> > +     "ok")))
> > +@result{} "ok"
> > +@end example
> You could inline 'obj' here -- unless there is a bug, match doesn't evaluate
> its ‘argument’ multiple times.

I am not aware of any bugs here, but I wanted to adhere to the style of the
original examples:

    (let ((l '(hello (world))))
      (match l           ;; <- the input object
        (('hello (who))  ;; <- the pattern
         who)))          ;; <- the expression evaluated upon matching

I can inline it if you would prefer.

> 
> > +
> > +If you need to ensure that a value is of a specific record type and at
> > +the same time bind it to a variable, the record operator will not be
> > +enough by itself, since you can only capture the fields.  You would need
> > +to combine it with other patterns, for example @code{(? foo? obj)}.
> > +
> > +When you need to apply multiple patterns, or a check and a pattern, you
> > +can use (@code{(? predicate pat_1 ... pat_n)}) for that.  The patterns
> > +are evaluated is if in the @code{(and ...)}.
> That last sentence needs a grammar check.

Right, typo corrected.

> 
> > If, for example, you want
> > +to check something is a symbol and at the same time bind the value to a
> > +variable, it could look like this:
> s/something/if something
> 
> or
> 
> s/something/whether something
> 
> > +
> > +@example
> > +(let ()
> > +  (match '(delete . some-id)
> > +    (('delete . (? symbol? id))
> > +     ;; We now could, for example, use the id to delete from some alist.
> > +     id)))
> Slightly misleading: while in this particular example you don't need it, you
> can also use ‘and’ for that.  Especially useful if there is no predicate in
> play/if the predicate would be (const #true).

I think I understand what you mean, but I was not sure how to reword it to make
it less misleading.  So I instead added one more paragraph exonerating the (and
...).

> 
> Whereas the previous 'let' might just be a matter of personal style, this
> let serves no purpose at all.  I would eliminate this distraction to
> eliminate the tiny risk that someone might be confused by it.

The "a more complex example" uses the (let () ...) form, so I just did the same.
Now I realize it is used to scope the (define-record-type).  I removed the let
from here and from few other places where it was not required.

> 
> > +@result{} some-id
> > +@end example
> > +
> > +@c FIXME: Remove this remark once everything is clearly described and
> > +@c consulting the comment is no longer necessary.
> > +If you are unclear about how something works, you can try consulting the
> > +large comment at the top of the @code{module/ice-9/match.upstream.scm}
> > +file in your guile distribution.
> s/guile/Guile.
> 
> That's not supposed to be there, but given the current (lack of) (ice-9
> match) documentation, I suppose it's reasonable.
> 
> Best regards,
> Maxime.

Have a nice day,

Tomas

-- 
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.