Re: Hygienic rewrite of (ice-9 expect)

[Daniel Dinnyes <dinnyesd@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3558 bytes --]

Hi Maxime,

Apologies for the 3 month delay in the reply, but it took some time to
until I managed to finish significant changes/rewrite to address your
points.

The new code is on the same branch, but rebased on current savannah master,
and significantly different from the earlier.

Please find my response to your points in red below:

On Wed, 5 Jul 2023 at 12:57, Maxime Devos <maximedevos@telenet.be> wrote:

>
>
> Op 19-06-2023 om 01:37 schreef Daniel Dinnyes:
> > I have to apologise for the massive hatched-job I did with that
> "rebase"! :P
> > Some references to my own modules were left in, other variables mixed up,
> > etc.
> >
> > I think it's in better shape now. Checked that tests run at least!
> > It's the same feature branch URL
> > <https://github.com/dadinn/guile/tree/wip-hygienic-expect>... but I've
> > force pushed it! ;)
>
>
> Some remarks:
>
> 1. The first commit 'added expect module' should instead be named
> 'Add incomplete hygienic expect module', because:
>
> 1(a) in Guile, the convention is to use the active voice in commit
> messages
>
Done

> 1(b) There is already an expect module; the hygiene bit is the difference.
>
Done

> 1(c) There are some missing bits, e.g. ‘use defaults for undefined
> parameter objects’ in (guile)Expect.
>
> Likewise, the other commits need some changes in commit message.
>
 Done

>
> 1. The first commit and ‘Added original attribution comments’
> could be squashed together -- I don't see a reason to have temporarily
> missing attribution information.

2. Likewise, the first commit can be squashed with parts of ‘Added
> original comments’.
>

Squashed them together, but the macros to which the doc-strings apply don't
exist in the initial commit.


>
> 3. For ‘use defaults for undefined parameter bindings': Guile has a
> thing named 'parameters' (search for make-parameter etc.), which isn't
> what is used here.  I recommend searching for other terminology.
>
> Also, for clarity, I would add to the commit message something like: ‘This
> ‘Nowadays something like this would be implemented with parameter
> objects or syntax-parameterize, but that would be a backwards
> incompatible change.’.
>

This is where the new code has the great difference compared to the earlier
implementation.
I've rewritten the core expect-with-bindings macro, such that instead of
juggling with passing arguments around in that recursive state-machine
macro, it now uses these module-internal named parameters.
Regarding backwards compatibility passing these parameters via lexical
binding takes priority over parameterizing named the named parameters.
To ensure compatibility, the named parameter bindings are not exported, and
could be used only via module-internal references like in here
<https://github.com/dadinn/guile/blob/wip-hygienic-expect/module/ice-9/expect-test.scm#L74>
.

>
> 4. In ‘Added interact macro implementation’, there is:
>
>   > +(define interact-expect-port-error
>   > +)
>
>     that should be on a single line instead IMO.
>

That line must have got there by accident. Removed it.


> 5. I haven't directly compared the (ice-9 expect) with the (ice-9
> expect) yet, but it should now be simple to verify (and anyway, there
> are tests).
>

Added improved test cases for the new interact macro, and also for using
named parameters instead of lexical binding.


>
> Best regards,
> Maxime Devos.
>


-- 
Best regards,
Daniel Dinnyes

[-- Attachment #2: Type: text/html, Size: 6244 bytes --]