Hygienic rewrite of (ice-9 expect)

Hygienic rewrite of (ice-9 expect)

[Daniel Dinnyes]

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

Hi everyone,

I am new to this mailing list, and writing this because I have encountered
some issues with ice-9 expect
<https://git.savannah.gnu.org/cgit/guile.git/tree/module/ice-9/expect.scm>:

The procedural expect macro has a serious problem:

This syntax expects a procedure as the test expression, with 2 arguments
(actual content read, and boolean EOF flag). The primary problem is that
when the test expression is not a identifier bound to such a procedure, but
instead an expression which creates such a procedure (aka "factory"), then
the expression will be evaluated anew for each character read from
expect-port. This is a serious problem: when constructing that procedure
has significant computational costs, or side effects (eg. opening a port to
a new log file), as it can be seen in my code here
<https://github.com/dadinn/system-setup-tests/blob/expect-bug/run.scm#L476>.
(This is a branch of some of my KVM-based E2E testing code, which uses the
original expect macro implementation, just to demonstrate the problem).

I had written a workaround for this problem, by binding the evaluation of
the test expression outside the expansion of the expect macro (as it can be
seen here
<https://github.com/dadinn/system-setup-tests/blob/master/run.scm#L29>).
The problem with this was, that it doesn't support the full capabilities of
the original expect macro, and debilitates it by only allowing for a single
conditional branch. Also, I haven't verified, but I suspicious it would
possibly even break the behaviour for the => syntax of the expect-strings
macro.

To implement a more complete version of this (and also as an exercise to
understand hygienic macros), I've attempted to rewrite my workaround using
syntax-case. I've ran into some issues, and my implementation didn't expand
as I expected. After some discussion on IRC, we've concluded
<http://snailgeist.com/irc-logs/?message_id=136306> that this was because
the current (ice-9 expect) implementation is written using unhygienic
defmacro syntax, which doesn't work well together with the hygienic
syntax-case. It was suggested I should rather rewrite expect completely to
be hygienic instead.

I've eventually completed the rewrite using syntax-case here
<https://github.com/dadinn/common/blob/feature/expect/expect.scm>, and
would be happy to contribute it back to the ice-9 module!
I would point out the following about this implementation:

   1. It works!!! I had some quite extensive E2E testing code written
   before here
   <https://github.com/dadinn/system-setup-tests/blob/develop/run.scm>,
   which I think really stretches the possibilities with (ice-9 expect).
   Switching to the new implementation was simple, and works flawlessly!
   2. It is backwards compatible! I've gone extra lengths to make sure the
   code would be a backwards compatible, a drop-in replacement.
   3. Introduced new feature for the procedural expect macro, such that the
   second argument of the procedure gets passed the currently read char, or #f
   if it was an EOF character. There is multiple reasons this is superior to
   passing just a boolean eof? flag argument:
   1. the same logic can be implemented as with the eof? boolean, just by
         negating the condition
         2. passing the currently read char avoids having to do the
         inefficient operation to retrieve the currently read char by
cutting off
         the last character from the end of the content. Currently it
is necessary
         to do this, if the logic wants something additional to be
done with the
         current char (eg. as it can be seen in my code here
         <https://github.com/dadinn/system-setup-tests/blob/develop/run.scm#L200>
         )
         3. one such case of additional use is making the expect-char-proc
         property obsolete, as the predicate procedure is now able to
execute such
         logic too, besides other more advanced logging scenarios
(like it can be
         seen in my code here
         <https://github.com/dadinn/system-setup-tests/blob/develop/run.scm#L205>:
         here besides logging everything to STDOUT, and also to a main
log-file, I
         also log for each expect macro call the contents it tries to compare
         against into a separately named minor log-file (useful for
debugging), and
         also to ensure that we actually do not write to STDOUT when
using expect on
         multiple parallel processes)
         4. To ensure that everything stays backwards compatible, this new
         feature is only enabled if the new expect-pass-char? binding is
         set to #t in the lexical context (like here
         <https://github.com/dadinn/system-setup-tests/commit/21bddce3cd1c14f8834adaee4ff75f5b1b7a7b04>).
         Otherwise, by default this feature is disabled, and boolean eof?
         flag argument is passed just like before.
         4. There is a better support for using default value/expressions
   for the various parameters the macros depend on in their lexical contexts.
   The old macro had to export default values defined in its module, so as to
   ensure the defaults are available. This either clutters the module's global
   context where (ice-9 expect) is used, or the defaults are not available
   when using something like ((ice-9 expect) #:select (expect
   expect-string)).  The new defaults are calculated based on the lexical
   context where the macros are used, and expands into the correct default
   values when they are not defined. For example when expect-port is not
   defined in the lexical context, then the default value used is going to be
   the result of the (current-input-port) expression. I would like someone
   to review whether I've implemented this correctly, as it seems the
   current-input-port identifier default for expect-port gets captured in
   the expansion. Not sure why that happens.
   5. The expect-strings macro supports the => syntax, which passes the
   output(s) of the test predicate to a procedure, instead of just evaluating
   a body. This is fully supported, but additionally, the => syntax now
   also works with the procedural expect macro too! I haven't verified if the
   original implementation did this too, but at least it wasn't described in
   the documentation!
   6. As an added bonus, I have added a simplistic implementation for an
   interact macro. It uses threads to read from STDIO, and expects expect-port
   to be an input-output-port, so that it can output the lines read from STDIO
   into the same port. Not too advanced, as it doesn't support a full terminal
   interface, with autocomplete, etc... but works well enough for my use-case
   to interact with a KVM process via serial port.
   7. mnieper said <http://snailgeist.com/irc-logs/?message_id=136211> that
   I am not using syntax-case idiomatically, and my style is more in the form
   of a syntax-rules-based state-machine.
   I am quite happy with how it is currently, but would be open to be
   convinced if there is a superior way of expressing all this.
   8. I currently have various tests sprinkled around my implementation, to
   manually demonstrate expansions, and executions.
   Also, I have added some automated unit tests for the bind-locally helper
   procedure, which I used for checking whether parameters are defined in the
   lexical scope.
   9. I am not happy with how I am managing my project structure, and how
   my modules can reference each other (having to call add-to-load-path
   everywhere).
   Also, I think the way I am using srfi-64 tests is a bit bothersome, as
   they always get executed when my modules are loaded, and I have to globally
   disable logging to files.
   I would like some recommendations how to do this better!

If the community likes this implementation, I would be happy to
apply/rebase my changes onto the official repo, and then we can go from
there!

Best regards,
Daniel Dinnyes

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

Re: Hygienic rewrite of (ice-9 expect)

[Daniel Dinnyes]

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

Hi everyone,

It seems this hasn't garnered much attention.

Nevertheless, meanwhile I've realised the code had a few bugs, in
particular around the backwards-compatibility flag.

I've done a major rewrite:

   - fixed couple of bugs I found in the original implementation
   - made implementation more robust, simpler, and backwards-compatible:
   - added new expect-chars macro to support the new
      non-backwards-compatible matcher procedure signature.
      - removed need for expect-pass-char? backwards-compatibility feature
      flag. expect and expect-strings macros work as in ice-9.
      - improved interact macro, to allow specifying separate expect/input
   and interact/output ports.

The code can be found here
<https://github.com/dadinn/common/blob/feature/expect4/expect.scm>.

Thanks,
Daniel


On Sat, 15 Apr 2023 at 16:10, Daniel Dinnyes <dinnyesd@gmail.com> wrote:

> Hi everyone,
>
> I am new to this mailing list, and writing this because I have encountered
> some issues with ice-9 expect
> <https://git.savannah.gnu.org/cgit/guile.git/tree/module/ice-9/expect.scm>
> :
>
> The procedural expect macro has a serious problem:
>
> This syntax expects a procedure as the test expression, with 2 arguments
> (actual content read, and boolean EOF flag). The primary problem is that
> when the test expression is not a identifier bound to such a procedure, but
> instead an expression which creates such a procedure (aka "factory"), then
> the expression will be evaluated anew for each character read from
> expect-port. This is a serious problem: when constructing that procedure
> has significant computational costs, or side effects (eg. opening a port to
> a new log file), as it can be seen in my code here
> <https://github.com/dadinn/system-setup-tests/blob/expect-bug/run.scm#L476>.
> (This is a branch of some of my KVM-based E2E testing code, which uses the
> original expect macro implementation, just to demonstrate the problem).
>
> I had written a workaround for this problem, by binding the evaluation of
> the test expression outside the expansion of the expect macro (as it can be
> seen here
> <https://github.com/dadinn/system-setup-tests/blob/master/run.scm#L29>).
> The problem with this was, that it doesn't support the full capabilities of
> the original expect macro, and debilitates it by only allowing for a single
> conditional branch. Also, I haven't verified, but I suspicious it would
> possibly even break the behaviour for the => syntax of the expect-strings
> macro.
>
> To implement a more complete version of this (and also as an exercise to
> understand hygienic macros), I've attempted to rewrite my workaround using
> syntax-case. I've ran into some issues, and my implementation didn't
> expand as I expected. After some discussion on IRC, we've concluded
> <http://snailgeist.com/irc-logs/?message_id=136306> that this was because
> the current (ice-9 expect) implementation is written using unhygienic
> defmacro syntax, which doesn't work well together with the hygienic
> syntax-case. It was suggested I should rather rewrite expect completely
> to be hygienic instead.
>
> I've eventually completed the rewrite using syntax-case here
> <https://github.com/dadinn/common/blob/feature/expect/expect.scm>, and
> would be happy to contribute it back to the ice-9 module!
> I would point out the following about this implementation:
>
>    1. It works!!! I had some quite extensive E2E testing code written
>    before here
>    <https://github.com/dadinn/system-setup-tests/blob/develop/run.scm>,
>    which I think really stretches the possibilities with (ice-9 expect).
>    Switching to the new implementation was simple, and works flawlessly!
>    2. It is backwards compatible! I've gone extra lengths to make sure
>    the code would be a backwards compatible, a drop-in replacement.
>    3. Introduced new feature for the procedural expect macro, such that
>    the second argument of the procedure gets passed the currently read char,
>    or #f if it was an EOF character. There is multiple reasons this is
>    superior to passing just a boolean eof? flag argument:
>    1. the same logic can be implemented as with the eof? boolean, just by
>          negating the condition
>          2. passing the currently read char avoids having to do the
>          inefficient operation to retrieve the currently read char by cutting off
>          the last character from the end of the content. Currently it is necessary
>          to do this, if the logic wants something additional to be done with the
>          current char (eg. as it can be seen in my code here
>          <https://github.com/dadinn/system-setup-tests/blob/develop/run.scm#L200>
>          )
>          3. one such case of additional use is making the expect-char-proc
>          property obsolete, as the predicate procedure is now able to execute such
>          logic too, besides other more advanced logging scenarios (like it can be
>          seen in my code here
>          <https://github.com/dadinn/system-setup-tests/blob/develop/run.scm#L205>:
>          here besides logging everything to STDOUT, and also to a main log-file, I
>          also log for each expect macro call the contents it tries to compare
>          against into a separately named minor log-file (useful for debugging), and
>          also to ensure that we actually do not write to STDOUT when using expect on
>          multiple parallel processes)
>          4. To ensure that everything stays backwards compatible, this
>          new feature is only enabled if the new expect-pass-char? binding
>          is set to #t in the lexical context (like here
>          <https://github.com/dadinn/system-setup-tests/commit/21bddce3cd1c14f8834adaee4ff75f5b1b7a7b04>).
>          Otherwise, by default this feature is disabled, and boolean eof?
>          flag argument is passed just like before.
>          4. There is a better support for using default value/expressions
>    for the various parameters the macros depend on in their lexical contexts.
>    The old macro had to export default values defined in its module, so as to
>    ensure the defaults are available. This either clutters the module's global
>    context where (ice-9 expect) is used, or the defaults are not
>    available when using something like ((ice-9 expect) #:select (expect
>    expect-string)).  The new defaults are calculated based on the lexical
>    context where the macros are used, and expands into the correct default
>    values when they are not defined. For example when expect-port is not
>    defined in the lexical context, then the default value used is going to be
>    the result of the (current-input-port) expression. I would like
>    someone to review whether I've implemented this correctly, as it seems the
>    current-input-port identifier default for expect-port gets captured in
>    the expansion. Not sure why that happens.
>    5. The expect-strings macro supports the => syntax, which passes the
>    output(s) of the test predicate to a procedure, instead of just evaluating
>    a body. This is fully supported, but additionally, the => syntax now
>    also works with the procedural expect macro too! I haven't verified if the
>    original implementation did this too, but at least it wasn't described in
>    the documentation!
>    6. As an added bonus, I have added a simplistic implementation for an
>    interact macro. It uses threads to read from STDIO, and expects expect-port
>    to be an input-output-port, so that it can output the lines read from STDIO
>    into the same port. Not too advanced, as it doesn't support a full terminal
>    interface, with autocomplete, etc... but works well enough for my use-case
>    to interact with a KVM process via serial port.
>    7. mnieper said <http://snailgeist.com/irc-logs/?message_id=136211>
>    that I am not using syntax-case idiomatically, and my style is more in the
>    form of a syntax-rules-based state-machine.
>    I am quite happy with how it is currently, but would be open to be
>    convinced if there is a superior way of expressing all this.
>    8. I currently have various tests sprinkled around my implementation,
>    to manually demonstrate expansions, and executions.
>    Also, I have added some automated unit tests for the bind-locally
>    helper procedure, which I used for checking whether parameters are defined
>    in the lexical scope.
>    9. I am not happy with how I am managing my project structure, and how
>    my modules can reference each other (having to call add-to-load-path
>    everywhere).
>    Also, I think the way I am using srfi-64 tests is a bit bothersome, as
>    they always get executed when my modules are loaded, and I have to globally
>    disable logging to files.
>    I would like some recommendations how to do this better!
>
> If the community likes this implementation, I would be happy to
> apply/rebase my changes onto the official repo, and then we can go from
> there!
>
> Best regards,
> Daniel Dinnyes
>
>
>

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

Re: Hygienic rewrite of (ice-9 expect)

[Daniel Dinnyes]

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

Need to add a correction:

The new interact macro captures the expect-port for input if available, and
uses it as the interact/output port too.
This is unless an explicit interact/output port is passed as an optional
argument, in which case the lines read from default current-input-port
(using ice-9 readline) are then sent to this port instead.

Cheers,
Daniel

On Sun, 21 May 2023 at 23:26, Daniel Dinnyes <dinnyesd@gmail.com> wrote:

> Hi everyone,
>
> It seems this hasn't garnered much attention.
>
> Nevertheless, meanwhile I've realised the code had a few bugs, in
> particular around the backwards-compatibility flag.
>
> I've done a major rewrite:
>
>    - fixed couple of bugs I found in the original implementation
>    - made implementation more robust, simpler, and backwards-compatible:
>    - added new expect-chars macro to support the new
>       non-backwards-compatible matcher procedure signature.
>       - removed need for expect-pass-char? backwards-compatibility
>       feature flag. expect and expect-strings macros work as in ice-9.
>       - improved interact macro, to allow specifying separate
>    expect/input and interact/output ports.
>
> The code can be found here
> <https://github.com/dadinn/common/blob/feature/expect4/expect.scm>.
>
> Thanks,
> Daniel
>
>
> On Sat, 15 Apr 2023 at 16:10, Daniel Dinnyes <dinnyesd@gmail.com> wrote:
>
>> Hi everyone,
>>
>> I am new to this mailing list, and writing this because I have
>> encountered some issues with ice-9 expect
>> <https://git.savannah.gnu.org/cgit/guile.git/tree/module/ice-9/expect.scm>
>> :
>>
>> The procedural expect macro has a serious problem:
>>
>> This syntax expects a procedure as the test expression, with 2 arguments
>> (actual content read, and boolean EOF flag). The primary problem is that
>> when the test expression is not a identifier bound to such a procedure, but
>> instead an expression which creates such a procedure (aka "factory"), then
>> the expression will be evaluated anew for each character read from
>> expect-port. This is a serious problem: when constructing that procedure
>> has significant computational costs, or side effects (eg. opening a port to
>> a new log file), as it can be seen in my code here
>> <https://github.com/dadinn/system-setup-tests/blob/expect-bug/run.scm#L476>.
>> (This is a branch of some of my KVM-based E2E testing code, which uses the
>> original expect macro implementation, just to demonstrate the problem).
>>
>> I had written a workaround for this problem, by binding the evaluation of
>> the test expression outside the expansion of the expect macro (as it can be
>> seen here
>> <https://github.com/dadinn/system-setup-tests/blob/master/run.scm#L29>).
>> The problem with this was, that it doesn't support the full capabilities of
>> the original expect macro, and debilitates it by only allowing for a single
>> conditional branch. Also, I haven't verified, but I suspicious it would
>> possibly even break the behaviour for the => syntax of the expect-strings
>> macro.
>>
>> To implement a more complete version of this (and also as an exercise to
>> understand hygienic macros), I've attempted to rewrite my workaround using
>> syntax-case. I've ran into some issues, and my implementation didn't
>> expand as I expected. After some discussion on IRC, we've concluded
>> <http://snailgeist.com/irc-logs/?message_id=136306> that this was
>> because the current (ice-9 expect) implementation is written using
>> unhygienic defmacro syntax, which doesn't work well together with the
>> hygienic syntax-case. It was suggested I should rather rewrite expect
>> completely to be hygienic instead.
>>
>> I've eventually completed the rewrite using syntax-case here
>> <https://github.com/dadinn/common/blob/feature/expect/expect.scm>, and
>> would be happy to contribute it back to the ice-9 module!
>> I would point out the following about this implementation:
>>
>>    1. It works!!! I had some quite extensive E2E testing code written
>>    before here
>>    <https://github.com/dadinn/system-setup-tests/blob/develop/run.scm>,
>>    which I think really stretches the possibilities with (ice-9 expect).
>>    Switching to the new implementation was simple, and works flawlessly!
>>    2. It is backwards compatible! I've gone extra lengths to make sure
>>    the code would be a backwards compatible, a drop-in replacement.
>>    3. Introduced new feature for the procedural expect macro, such that
>>    the second argument of the procedure gets passed the currently read char,
>>    or #f if it was an EOF character. There is multiple reasons this is
>>    superior to passing just a boolean eof? flag argument:
>>    1. the same logic can be implemented as with the eof? boolean, just
>>          by negating the condition
>>          2. passing the currently read char avoids having to do the
>>          inefficient operation to retrieve the currently read char by cutting off
>>          the last character from the end of the content. Currently it is necessary
>>          to do this, if the logic wants something additional to be done with the
>>          current char (eg. as it can be seen in my code here
>>          <https://github.com/dadinn/system-setup-tests/blob/develop/run.scm#L200>
>>          )
>>          3. one such case of additional use is making the
>>          expect-char-proc property obsolete, as the predicate procedure
>>          is now able to execute such logic too, besides other more advanced logging
>>          scenarios (like it can be seen in my code here
>>          <https://github.com/dadinn/system-setup-tests/blob/develop/run.scm#L205>:
>>          here besides logging everything to STDOUT, and also to a main log-file, I
>>          also log for each expect macro call the contents it tries to compare
>>          against into a separately named minor log-file (useful for debugging), and
>>          also to ensure that we actually do not write to STDOUT when using expect on
>>          multiple parallel processes)
>>          4. To ensure that everything stays backwards compatible, this
>>          new feature is only enabled if the new expect-pass-char?
>>          binding is set to #t in the lexical context (like here
>>          <https://github.com/dadinn/system-setup-tests/commit/21bddce3cd1c14f8834adaee4ff75f5b1b7a7b04>).
>>          Otherwise, by default this feature is disabled, and boolean eof?
>>          flag argument is passed just like before.
>>          4. There is a better support for using default
>>    value/expressions for the various parameters the macros depend on in their
>>    lexical contexts. The old macro had to export default values defined in its
>>    module, so as to ensure the defaults are available. This either clutters
>>    the module's global context where (ice-9 expect) is used, or the
>>    defaults are not available when using something like ((ice-9 expect)
>>    #:select (expect expect-string)).  The new defaults are calculated
>>    based on the lexical context where the macros are used, and expands into
>>    the correct default values when they are not defined. For example when
>>    expect-port is not defined in the lexical context, then the default value
>>    used is going to be the result of the (current-input-port)
>>    expression. I would like someone to review whether I've implemented this
>>    correctly, as it seems the current-input-port identifier default for
>>    expect-port gets captured in the expansion. Not sure why that happens.
>>    5. The expect-strings macro supports the => syntax, which passes the
>>    output(s) of the test predicate to a procedure, instead of just evaluating
>>    a body. This is fully supported, but additionally, the => syntax now
>>    also works with the procedural expect macro too! I haven't verified if the
>>    original implementation did this too, but at least it wasn't described in
>>    the documentation!
>>    6. As an added bonus, I have added a simplistic implementation for an
>>    interact macro. It uses threads to read from STDIO, and expects expect-port
>>    to be an input-output-port, so that it can output the lines read from STDIO
>>    into the same port. Not too advanced, as it doesn't support a full terminal
>>    interface, with autocomplete, etc... but works well enough for my use-case
>>    to interact with a KVM process via serial port.
>>    7. mnieper said <http://snailgeist.com/irc-logs/?message_id=136211>
>>    that I am not using syntax-case idiomatically, and my style is more in the
>>    form of a syntax-rules-based state-machine.
>>    I am quite happy with how it is currently, but would be open to be
>>    convinced if there is a superior way of expressing all this.
>>    8. I currently have various tests sprinkled around my implementation,
>>    to manually demonstrate expansions, and executions.
>>    Also, I have added some automated unit tests for the bind-locally
>>    helper procedure, which I used for checking whether parameters are defined
>>    in the lexical scope.
>>    9. I am not happy with how I am managing my project structure, and
>>    how my modules can reference each other (having to call add-to-load-path
>>    everywhere).
>>    Also, I think the way I am using srfi-64 tests is a bit bothersome,
>>    as they always get executed when my modules are loaded, and I have to
>>    globally disable logging to files.
>>    I would like some recommendations how to do this better!
>>
>> If the community likes this implementation, I would be happy to
>> apply/rebase my changes onto the official repo, and then we can go from
>> there!
>>
>> Best regards,
>> Daniel Dinnyes
>>
>>
>>

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

Re: Hygienic rewrite of (ice-9 expect)

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 2011 bytes --]

I think this would gather more replies if:

  1. It is sent as a patch that can be applied to the Guile source tree.

(You wrote:

 > If the community likes this implementation, I would be happy to
 > apply/rebase my changes onto the official repo, and then we can go
 > from there!

but to determine whether I'm happy with it, it would be convenient if 
this were a proper patch.)
(I'm not actually reviewing Guile stuff at the moment, but I find it 
likely that some others might hold the same view.)

  2. It is split into two patches: a patch that only does the hygienic
     rewrite, and an additional patch that adds the new features.

     Then people who are interested in the patch for the hygiene but
     don't really grok (or are uninterested in, or don't have time for)
     the new features can ACK or NACK the first patch without having to
     worry about the second patch, and additionally it becomes easier to
     verify there are no new bugs.

  3. You respect the LGPLv3.0.  The LGPLv3.0 requires you to follow
     most terms of the GPLv3.0, but you didn't follow 5(a),
     4(a) ‘keep intact all notices’ and 4(a) ‘give all recipients a copy
     of this License’ -- the LPGLv3 is more than only the LICENSE file
     you included; it includes parts of the GPLv3 by reference.

     Also, you only put the LGPLv3.0 text, without specifying a version
     number. Then by ‘6. Revised Versions of this License’ of the LGPL:

 > If the Library as you received it does not specify a version number of
 > the GNU Lesser General Public License, you may choose any version of
 > the GNU Lesser General Public License ever published by the Free
 > Software Foundation.

     you are licensing your implementation as LGPLv2.0+, but
     that's illegal, because your implementation is a rewrite of Guile's
     and hence most likely a derivative work of Guile 3.0, and
     Guile 3.0 is LGPLv3+ licensed, not LGPLv2+.

Best regards,
Maxime.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: Hygienic rewrite of (ice-9 expect)

[Daniel Dinnyes]

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

Thanks for the reply.

On Sun, 28 May 2023 at 14:39, Maxime Devos <maximedevos@telenet.be> wrote:

> I think this would gather more replies if:
>
>   1. It is sent as a patch that can be applied to the Guile source tree.
>

(You wrote:
>
>  > If the community likes this implementation, I would be happy to
>  > apply/rebase my changes onto the official repo, and then we can go
>  > from there!
>
> but to determine whether I'm happy with it, it would be convenient if
> this were a proper patch.)
> (I'm not actually reviewing Guile stuff at the moment, but I find it
> likely that some others might hold the same view.)
>
>
Regarding patches, I think I've read somewhere that GNU wants contributions
in some different format,
than just URLs to pull-able GIT repos. Could you share with me some guide
how to do such patches,
or what format they are needed?

I am assuming this <https://git.savannah.gnu.org/cgit/guile.git/> is the
repo of current guile tree, onto which to apply my changes.
Onto which branch?

Would it possible to fork the repo on savannah, and then raise a PR from
there instead of patches?
Also, I've tried to create account on savannah, but it keeps getting
deleted.
What would you recommend I do to ensure my account doesn't get removed?


>   2. It is split into two patches: a patch that only does the hygienic
>      rewrite, and an additional patch that adds the new features.
>
>      Then people who are interested in the patch for the hygiene but
>      don't really grok (or are uninterested in, or don't have time for)
>      the new features can ACK or NACK the first patch without having to
>      worry about the second patch, and additionally it becomes easier to
>      verify there are no new bugs.
>

No probs, would do so!


>   3. You respect the LGPLv3.0.  The LGPLv3.0 requires you to follow
>      most terms of the GPLv3.0, but you didn't follow 5(a),
>      4(a) ‘keep intact all notices’ and 4(a) ‘give all recipients a copy
>      of this License’ -- the LPGLv3 is more than only the LICENSE file
>      you included; it includes parts of the GPLv3 by reference.
>
>      Also, you only put the LGPLv3.0 text, without specifying a version
>      number. Then by ‘6. Revised Versions of this License’ of the LGPL:
>
>  > If the Library as you received it does not specify a version number of
>  > the GNU Lesser General Public License, you may choose any version of
>  > the GNU Lesser General Public License ever published by the Free
>  > Software Foundation.
>
>      you are licensing your implementation as LGPLv2.0+, but
>      that's illegal, because your implementation is a rewrite of Guile's
>      and hence most likely a derivative work of Guile 3.0, and
>      Guile 3.0 is LGPLv3+ licensed, not LGPLv2+.
>
>
I didn't notice that I've added LGPL licence instead of GPL! If you look at
my other code which uses this module,
those are all GPLv3. It could be that I've realised that Guile itself is
under LGPL? Not sure, I can't recall it!
Nevertheless, this licence itself is LGPLv3, not v2!

I am not sure I would agree with your assessment about *illegality*, and
the *derivative work* categorization.
Even though these macros happen to expand to something similar as the
original ice-9 implementation,
the code itself is quite significantly different! If this is to be used in
ice-9, it would have to completely
replace the original expect.scm file, as nothing was copy/pasted from
there. The fact that parameter binding
names are the same is necessary for backwards compatibility, so those
wouldn't count even under the US laws
<https://www.bbc.co.uk/news/technology-56639088>.

On second thought, I am wrong! The expect-select helper function looks like
a direct copy-paste job... little naughty me!

Nevertheless, I would be happy to add the necessary notices if that is
required.
Also, IIRC there would be another copyright assignment administrative work
somewhere down the line?

Best regards,
Daniel

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

Re: Hygienic rewrite of (ice-9 expect)

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 4883 bytes --]

 >p 29-05-2023 om 03:40 schreef Daniel Dinnyes:
> Thanks for the reply.
> 
> On Sun, 28 May 2023 at 14:39, Maxime Devos <maximedevos@telenet.be 
> <mailto:maximedevos@telenet.be>> wrote:
> 
>     I think this would gather more replies if:
> 
>        1. It is sent as a patch that can be applied to the Guile source
>     tree.
> 
>     (You wrote:
> 
>       > If the community likes this implementation, I would be happy to
>       > apply/rebase my changes onto the official repo, and then we can go
>       > from there!
> 
>     but to determine whether I'm happy with it, it would be convenient if
>     this were a proper patch.)
>     (I'm not actually reviewing Guile stuff at the moment, but I find it
>     likely that some others might hold the same view.)
> 
> 
> Regarding patches, I think I've read somewhere that GNU wants 
> contributions in some different format,
> than just URLs to pull-able GIT repos. Could you share with me some 
> guide how to do such patches,
> or what format they are needed?

Format: the result of 'git send-email', or 'git format-patch + attach 
the patch as an attachment'.  I've heard that the guide at 
<https://git-send-email.io/> for 'git send-email' is good.

Still, setting up a fork + sending a PR (by e-mail), while not the most 
preferred format, would still be pretty good (‘git diff’ can then be used!).

> I am assuming this <https://git.savannah.gnu.org/cgit/guile.git/> is the 
> repo of current guile tree, onto which to apply my changes.

Yes.

> Onto which branch?

The branch named 'main'.

> Would it possible to fork the repo on savannah, and then raise a PR from 
> there instead of patches?

Savannah doesn't have a notion of 'forks' AFAIK. (Emphasis on the ‘I 
don't know’ in ‘AFAIK’, maybe it actually does have them in some form.)

However, you can do a PR ‘manually’ by setting up a fork of the repo at 
some random hosting site of your choice, pushing some commits there, and 
sending a (free-form) e-mail to guile-devel@gnu.org with a message 
containing information on where to find the repository and which 
branch/commit.

(You don't need PR / fork buttons to do PRs and forks!)

> Also, I've tried to create account on savannah, but it keeps getting 
> deleted.
> What would you recommend I do to ensure my account doesn't get removed?

I don't have clue; I only fetch from savannah, I don't have an account 
there.

> [...] 
> I am not sure I would agree with your assessment about /illegality/, and 
> the /derivative work/ categorization.
> Even though these macros happen to expand to something similar as the 
> original ice-9 implementation,
> the code itself is quite significantly different! If this is to be used 
> in ice-9, it would have to completely
> replace the original expect.scm file, as nothing was copy/pasted from 
> there. The fact that parameter binding
> names are the same is necessary for backwards compatibility, so those 
> wouldn't count even under the US laws 
> <https://www.bbc.co.uk/news/technology-56639088>.

I assumed it is a derivative work because you presented it as a ‘rewrite 
of [the original]’.  Maybe it's possible to rewrite something a lot 
until its no longer a derivative work (I don't know if that's possible), 
but by default I'd assume that rewrites are derivative works.

I didn't use ‘same procedure names’ as a reason (as you wrote, those 
aren't a problem).

> On second thought, I am wrong! The expect-select helper function looks 
> like a direct copy-paste job... little naughty me!

TBC, I ascribe no guilt.  The thing is that I've noticed in the past 
that people often use the GPL without knowing what that entails 
precisely, which can have unintended consequences, like e.g. 
unintentionally licensing something as GPLv1+ instead of GPLv3+ (or 
GPLv3+ instead of GPLv3, but that mistake is actually pretty convenient 
for distributions :P).

(Also, Guile relies on the (L)GPL as a form of protection against some 
forms of malice, so I think it's important that we also follow the 
(L)GPL terms itself.)

> Nevertheless, I would be happy to add the necessary notices if that is 
> required.
> Also, IIRC there would be another copyright assignment administrative work
> somewhere down the line?

The copyright assignment used to be required, but nowadays its optional. 
  Still, it does appear to be preferred.  You would get an e-mail by a 
Guile maintainer with more info, it's not something you initiate yourself.

The page https://www.fsf.org/licensing/contributor-faq sort-of implies 
the opposite, but IIRC there is another page somewhere or gnu.org that 
doesn't; I don't know what's up with that.  (Either way, I'd think that 
things will work out in the end.)

Best regards,
Maxime Devos

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: Hygienic rewrite of (ice-9 expect)

[Daniel Dinnyes]

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

I've rebased my changes onto a fork of Guile's main branch on Github:
https://github.com/dadinn/guile/tree/wip-hygienic-expect

Added back the original attributions and comments.
I've also tried to follow the branch-naming conventions. 😉

Also, I've added a module ice-9/expect-tests for some comprehensive tests.
Wasn't sure where to place it, and how to integrate into the existing
test-suite.

Not sure how to send a PR by email, but I suppose it should be easy to
pull, by adding my fork as an additional remote.

Best regards,
Daniel Dinnyes


On Mon, 29 May 2023 at 22:33, Maxime Devos <maximedevos@telenet.be> wrote:

>  >p 29-05-2023 om 03:40 schreef Daniel Dinnyes:
> > Thanks for the reply.
> >
> > On Sun, 28 May 2023 at 14:39, Maxime Devos <maximedevos@telenet.be
> > <mailto:maximedevos@telenet.be>> wrote:
> >
> >     I think this would gather more replies if:
> >
> >        1. It is sent as a patch that can be applied to the Guile source
> >     tree.
> >
> >     (You wrote:
> >
> >       > If the community likes this implementation, I would be happy to
> >       > apply/rebase my changes onto the official repo, and then we can
> go
> >       > from there!
> >
> >     but to determine whether I'm happy with it, it would be convenient if
> >     this were a proper patch.)
> >     (I'm not actually reviewing Guile stuff at the moment, but I find it
> >     likely that some others might hold the same view.)
> >
> >
> > Regarding patches, I think I've read somewhere that GNU wants
> > contributions in some different format,
> > than just URLs to pull-able GIT repos. Could you share with me some
> > guide how to do such patches,
> > or what format they are needed?
>
> Format: the result of 'git send-email', or 'git format-patch + attach
> the patch as an attachment'.  I've heard that the guide at
> <https://git-send-email.io/> for 'git send-email' is good.
>
> Still, setting up a fork + sending a PR (by e-mail), while not the most
> preferred format, would still be pretty good (‘git diff’ can then be
> used!).
>
> > I am assuming this <https://git.savannah.gnu.org/cgit/guile.git/> is
> the
> > repo of current guile tree, onto which to apply my changes.
>
> Yes.
>
> > Onto which branch?
>
> The branch named 'main'.
>
> > Would it possible to fork the repo on savannah, and then raise a PR from
> > there instead of patches?
>
> Savannah doesn't have a notion of 'forks' AFAIK. (Emphasis on the ‘I
> don't know’ in ‘AFAIK’, maybe it actually does have them in some form.)
>
> However, you can do a PR ‘manually’ by setting up a fork of the repo at
> some random hosting site of your choice, pushing some commits there, and
> sending a (free-form) e-mail to guile-devel@gnu.org with a message
> containing information on where to find the repository and which
> branch/commit.
>
> (You don't need PR / fork buttons to do PRs and forks!)
>
> > Also, I've tried to create account on savannah, but it keeps getting
> > deleted.
> > What would you recommend I do to ensure my account doesn't get removed?
>
> I don't have clue; I only fetch from savannah, I don't have an account
> there.
>
> > [...]
> > I am not sure I would agree with your assessment about /illegality/, and
> > the /derivative work/ categorization.
> > Even though these macros happen to expand to something similar as the
> > original ice-9 implementation,
> > the code itself is quite significantly different! If this is to be used
> > in ice-9, it would have to completely
> > replace the original expect.scm file, as nothing was copy/pasted from
> > there. The fact that parameter binding
> > names are the same is necessary for backwards compatibility, so those
> > wouldn't count even under the US laws
> > <https://www.bbc.co.uk/news/technology-56639088>.
>
> I assumed it is a derivative work because you presented it as a ‘rewrite
> of [the original]’.  Maybe it's possible to rewrite something a lot
> until its no longer a derivative work (I don't know if that's possible),
> but by default I'd assume that rewrites are derivative works.
>
> I didn't use ‘same procedure names’ as a reason (as you wrote, those
> aren't a problem).
>
> > On second thought, I am wrong! The expect-select helper function looks
> > like a direct copy-paste job... little naughty me!
>
> TBC, I ascribe no guilt.  The thing is that I've noticed in the past
> that people often use the GPL without knowing what that entails
> precisely, which can have unintended consequences, like e.g.
> unintentionally licensing something as GPLv1+ instead of GPLv3+ (or
> GPLv3+ instead of GPLv3, but that mistake is actually pretty convenient
> for distributions :P).
>
> (Also, Guile relies on the (L)GPL as a form of protection against some
> forms of malice, so I think it's important that we also follow the
> (L)GPL terms itself.)
>
> > Nevertheless, I would be happy to add the necessary notices if that is
> > required.
> > Also, IIRC there would be another copyright assignment administrative
> work
> > somewhere down the line?
>
> The copyright assignment used to be required, but nowadays its optional.
>   Still, it does appear to be preferred.  You would get an e-mail by a
> Guile maintainer with more info, it's not something you initiate yourself.
>
> The page https://www.fsf.org/licensing/contributor-faq sort-of implies
> the opposite, but IIRC there is another page somewhere or gnu.org that
> doesn't; I don't know what's up with that.  (Either way, I'd think that
> things will work out in the end.)
>
> Best regards,
> Maxime Devos
>

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

Re: Hygienic rewrite of (ice-9 expect)

[Daniel Dinnyes]

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

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! ;)

Best regards,
Daniel Dinnyes

On Sun, 11 Jun 2023 at 16:48, Daniel Dinnyes <dinnyesd@gmail.com> wrote:

> I've rebased my changes onto a fork of Guile's main branch on Github:
> https://github.com/dadinn/guile/tree/wip-hygienic-expect
>
> Added back the original attributions and comments.
> I've also tried to follow the branch-naming conventions. 😉
>
> Also, I've added a module ice-9/expect-tests for some comprehensive tests.
> Wasn't sure where to place it, and how to integrate into the existing
> test-suite.
>
> Not sure how to send a PR by email, but I suppose it should be easy to
> pull, by adding my fork as an additional remote.
>
> Best regards,
> Daniel Dinnyes
>
>
> On Mon, 29 May 2023 at 22:33, Maxime Devos <maximedevos@telenet.be> wrote:
>
>>  >p 29-05-2023 om 03:40 schreef Daniel Dinnyes:
>> > Thanks for the reply.
>> >
>> > On Sun, 28 May 2023 at 14:39, Maxime Devos <maximedevos@telenet.be
>> > <mailto:maximedevos@telenet.be>> wrote:
>> >
>> >     I think this would gather more replies if:
>> >
>> >        1. It is sent as a patch that can be applied to the Guile source
>> >     tree.
>> >
>> >     (You wrote:
>> >
>> >       > If the community likes this implementation, I would be happy to
>> >       > apply/rebase my changes onto the official repo, and then we can
>> go
>> >       > from there!
>> >
>> >     but to determine whether I'm happy with it, it would be convenient
>> if
>> >     this were a proper patch.)
>> >     (I'm not actually reviewing Guile stuff at the moment, but I find it
>> >     likely that some others might hold the same view.)
>> >
>> >
>> > Regarding patches, I think I've read somewhere that GNU wants
>> > contributions in some different format,
>> > than just URLs to pull-able GIT repos. Could you share with me some
>> > guide how to do such patches,
>> > or what format they are needed?
>>
>> Format: the result of 'git send-email', or 'git format-patch + attach
>> the patch as an attachment'.  I've heard that the guide at
>> <https://git-send-email.io/> for 'git send-email' is good.
>>
>> Still, setting up a fork + sending a PR (by e-mail), while not the most
>> preferred format, would still be pretty good (‘git diff’ can then be
>> used!).
>>
>> > I am assuming this <https://git.savannah.gnu.org/cgit/guile.git/> is
>> the
>> > repo of current guile tree, onto which to apply my changes.
>>
>> Yes.
>>
>> > Onto which branch?
>>
>> The branch named 'main'.
>>
>> > Would it possible to fork the repo on savannah, and then raise a PR
>> from
>> > there instead of patches?
>>
>> Savannah doesn't have a notion of 'forks' AFAIK. (Emphasis on the ‘I
>> don't know’ in ‘AFAIK’, maybe it actually does have them in some form.)
>>
>> However, you can do a PR ‘manually’ by setting up a fork of the repo at
>> some random hosting site of your choice, pushing some commits there, and
>> sending a (free-form) e-mail to guile-devel@gnu.org with a message
>> containing information on where to find the repository and which
>> branch/commit.
>>
>> (You don't need PR / fork buttons to do PRs and forks!)
>>
>> > Also, I've tried to create account on savannah, but it keeps getting
>> > deleted.
>> > What would you recommend I do to ensure my account doesn't get removed?
>>
>> I don't have clue; I only fetch from savannah, I don't have an account
>> there.
>>
>> > [...]
>> > I am not sure I would agree with your assessment about /illegality/,
>> and
>> > the /derivative work/ categorization.
>> > Even though these macros happen to expand to something similar as the
>> > original ice-9 implementation,
>> > the code itself is quite significantly different! If this is to be used
>> > in ice-9, it would have to completely
>> > replace the original expect.scm file, as nothing was copy/pasted from
>> > there. The fact that parameter binding
>> > names are the same is necessary for backwards compatibility, so those
>> > wouldn't count even under the US laws
>> > <https://www.bbc.co.uk/news/technology-56639088>.
>>
>> I assumed it is a derivative work because you presented it as a ‘rewrite
>> of [the original]’.  Maybe it's possible to rewrite something a lot
>> until its no longer a derivative work (I don't know if that's possible),
>> but by default I'd assume that rewrites are derivative works.
>>
>> I didn't use ‘same procedure names’ as a reason (as you wrote, those
>> aren't a problem).
>>
>> > On second thought, I am wrong! The expect-select helper function looks
>> > like a direct copy-paste job... little naughty me!
>>
>> TBC, I ascribe no guilt.  The thing is that I've noticed in the past
>> that people often use the GPL without knowing what that entails
>> precisely, which can have unintended consequences, like e.g.
>> unintentionally licensing something as GPLv1+ instead of GPLv3+ (or
>> GPLv3+ instead of GPLv3, but that mistake is actually pretty convenient
>> for distributions :P).
>>
>> (Also, Guile relies on the (L)GPL as a form of protection against some
>> forms of malice, so I think it's important that we also follow the
>> (L)GPL terms itself.)
>>
>> > Nevertheless, I would be happy to add the necessary notices if that is
>> > required.
>> > Also, IIRC there would be another copyright assignment administrative
>> work
>> > somewhere down the line?
>>
>> The copyright assignment used to be required, but nowadays its optional.
>>   Still, it does appear to be preferred.  You would get an e-mail by a
>> Guile maintainer with more info, it's not something you initiate yourself.
>>
>> The page https://www.fsf.org/licensing/contributor-faq sort-of implies
>> the opposite, but IIRC there is another page somewhere or gnu.org that
>> doesn't; I don't know what's up with that.  (Either way, I'd think that
>> things will work out in the end.)
>>
>> Best regards,
>> Maxime Devos
>>
>

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

Re: Hygienic rewrite of (ice-9 expect)

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 75 bytes --]

Will look later ... have been busy. I expect to have time after ~2 weeks.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: Hygienic rewrite of (ice-9 expect)

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 1976 bytes --]



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
1(b) There is already an expect module; the hygiene bit is the difference.
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.

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

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

4. In ‘Added interact macro implementation’, there is:

  > +(define interact-expect-port-error
  > +)

    that should be on a single line instead IMO.

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

Best regards,
Maxime Devos.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: Hygienic rewrite of (ice-9 expect)

[Daniel Dinnyes]

[-- 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 --]

Re: Hygienic rewrite of (ice-9 expect)

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 454 bytes --]

Hi,

I received the latest message/version but I don't know what to do with 
it -- it seems an improvement wit some slight issues but no those are 
aren't actually issues and ... and, I get stuck in some vague loop of 
sorts and can't figure out what the maybe-issue is.

So, err, sorry for that, but I'm out of here and will leave further 
review/applying/... to the maintainers and random guile-devel visitors.

Best regards,
Maxime Devos.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: Hygienic rewrite of (ice-9 expect)

[Daniel Dinnyes]

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

Hey,

I've created a new branch/ref
<https://github.com/dadinn/guile/tree/wip-hygienic-expect2> with some
additional rewrite/changes I made.
Additionally, all was rebased on current master.

The main difference is that I changed the named parameters to use
**ear-muffed** syntax...
...an idiom in Clojure for thread local dynamically bound variables, which
named parameters are.
This way they don't conflict with the old/original lexically
scoped/captured parameters.
Therefore now the named parameters could be exported, and directly accessed
from the module.

Regarding, that vague loopy behaviour... I am not sure if you are referring
to the same thing, but I did encounter something similar with while running
the tests.
It seemed to me the issue happened non-deterministically while running the
interact macro tests.
I couldn't identify whether the issue was with the macro itself, or with
the test.
Interact runs on 2 threads: the main thread runs expect on the output port
of the process, while the other is reading user input and submitting it to
the process via its input port.
IMHO it is ensured that the interact thread gets closed on exit.
Also, I tried to make sure the tests close all ports correctly.
Not sure where the issue could be coming from.
At any rate, I have used the interact macro extensively outside of the
tests, and never had such problems with it.

Best regards,
Daniel Dinnyes

On Sat, 30 Sept 2023 at 21:22, Maxime Devos <maximedevos@telenet.be> wrote:

> Hi,
>
> I received the latest message/version but I don't know what to do with
> it -- it seems an improvement wit some slight issues but no those are
> aren't actually issues and ... and, I get stuck in some vague loop of
> sorts and can't figure out what the maybe-issue is.
>
> So, err, sorry for that, but I'm out of here and will leave further
> review/applying/... to the maintainers and random guile-devel visitors.
>
> Best regards,
> Maxime Devos.
>


-- 
Best regards,
Daniel Dinnyes

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

Re: Hygienic rewrite of (ice-9 expect)

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 458 bytes --]



Op 27-10-2023 om 03:03 schreef Daniel Dinnyes:
> Regarding, that vague loopy behaviour... I am not sure if you are 
> referring to the same thing, but I did encounter something similar with 
> while running the tests.

I don't think we are referring to the same thing -- I was referring to 
personally not being able to figure out what to do with the patches, I 
wasn't referring to some specific technical issue.

Best regards,
Maxime Devos.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: Hygienic rewrite of (ice-9 expect)

[Daniel Dinnyes]

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

Hi Maxime,

I am still not sure I understand what you meant.
You've said you weren't referring to technical issue, but also that you
"get stuck in some vague loop of sorts and can't figure out what the
maybe-issue is".

Regarding what to do with the "patches", I think a git pull should work, or
alternatively a rebase would be unlikely to cause conflicts either.

Could you please clarify?

Best regards,
Daniel Dinnyes

On Fri, 27 Oct 2023 at 17:58, Maxime Devos <maximedevos@telenet.be> wrote:

>
>
> Op 27-10-2023 om 03:03 schreef Daniel Dinnyes:
> > Regarding, that vague loopy behaviour... I am not sure if you are
> > referring to the same thing, but I did encounter something similar with
> > while running the tests.
>
> I don't think we are referring to the same thing -- I was referring to
> personally not being able to figure out what to do with the patches, I
> wasn't referring to some specific technical issue.
>
> Best regards,
> Maxime Devos.
>


-- 
Best regards,
Daniel Dinnyes

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

Re: Hygienic rewrite of (ice-9 expect)

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 612 bytes --]



Op 18-11-2023 om 12:29 schreef Daniel Dinnyes:
> Hi Maxime,
> 
> I am still not sure I understand what you meant.
> You've said you weren't referring to technical issue, but also that you 
> "get stuck in some vague loop of sorts and can't figure out what the 
> maybe-issue is".
> 
> Regarding what to do with the "patches", I think a git pull should work, 
> or alternatively a rebase would be unlikely to cause conflicts either.

That's not what I meant -- I meant how to review them and what to say 
about them.

> Could you please clarify?

No, my entire point is that I can't clarify.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: Hygienic rewrite of (ice-9 expect)

[Daniel Dinnyes]

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

Sure. Apologies for my confusion / misunderstanding.

On Sun, 19 Nov 2023 at 23:47, Maxime Devos <maximedevos@telenet.be> wrote:

>
>
> Op 18-11-2023 om 12:29 schreef Daniel Dinnyes:
> > Hi Maxime,
> >
> > I am still not sure I understand what you meant.
> > You've said you weren't referring to technical issue, but also that you
> > "get stuck in some vague loop of sorts and can't figure out what the
> > maybe-issue is".
> >
> > Regarding what to do with the "patches", I think a git pull should work,
> > or alternatively a rebase would be unlikely to cause conflicts either.
>
> That's not what I meant -- I meant how to review them and what to say
> about them.
>
> > Could you please clarify?
>
> No, my entire point is that I can't clarify.
>


-- 
Best regards,
Daniel Dinnyes

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

Re: Hygienic rewrite of (ice-9 expect)

[Daniel Dinnyes]

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

Hi everyone,

I am curious if anyone has interest in discussing this any further.

Best regards,
Daniel Dinnyes

On Sun, 19 Nov 2023 at 23:49, Daniel Dinnyes <dinnyesd@gmail.com> wrote:

> Sure. Apologies for my confusion / misunderstanding.
>
> On Sun, 19 Nov 2023 at 23:47, Maxime Devos <maximedevos@telenet.be> wrote:
>
>>
>>
>> Op 18-11-2023 om 12:29 schreef Daniel Dinnyes:
>> > Hi Maxime,
>> >
>> > I am still not sure I understand what you meant.
>> > You've said you weren't referring to technical issue, but also that you
>> > "get stuck in some vague loop of sorts and can't figure out what the
>> > maybe-issue is".
>> >
>> > Regarding what to do with the "patches", I think a git pull should
>> work,
>> > or alternatively a rebase would be unlikely to cause conflicts either.
>>
>> That's not what I meant -- I meant how to review them and what to say
>> about them.
>>
>> > Could you please clarify?
>>
>> No, my entire point is that I can't clarify.
>>
>
>
> --
> Best regards,
> Daniel Dinnyes
>


-- 
Best regards,
Daniel Dinnyes

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