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