From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Daniel Dinnyes Newsgroups: gmane.lisp.guile.devel Subject: Re: Hygienic rewrite of (ice-9 expect) Date: Sun, 24 Sep 2023 02:02:37 +0100 Message-ID: References: <428b8157-afec-03b9-f5f6-dfdd2f114131@telenet.be> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="0000000000000ab93e0606106846" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="36036"; mail-complaints-to="usenet@ciao.gmane.io" Cc: guile-devel@gnu.org To: Maxime Devos Original-X-From: guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org Sun Sep 24 03:03:47 2023 Return-path: Envelope-to: guile-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1qkDXG-00099X-Uj for guile-devel@m.gmane-mx.org; Sun, 24 Sep 2023 03:03:47 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qkDWs-00048V-RO; Sat, 23 Sep 2023 21:03:22 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qkDWq-000481-M2 for guile-devel@gnu.org; Sat, 23 Sep 2023 21:03:20 -0400 Original-Received: from mail-wr1-x430.google.com ([2a00:1450:4864:20::430]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qkDWn-0002LZ-EB for guile-devel@gnu.org; Sat, 23 Sep 2023 21:03:20 -0400 Original-Received: by mail-wr1-x430.google.com with SMTP id ffacd0b85a97d-3231eff8aecso299770f8f.1 for ; Sat, 23 Sep 2023 18:03:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695517395; x=1696122195; darn=gnu.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=xoPJblD6RR86hx7srJwcXnNzSH4ewmZToB9+JG9whdw=; b=ZfpEYDKYIyR665iP/Fym10fM8CvsnnX/J7PCMMIgEWqnnbhp3jLdjz/pzs2xH93Jc7 ke4yVxc67WudmiNdYPDqkYTlXP9rwVTUJsBwtuLEfZk08Y4egiLbo0vWygcWzR15yVdP rN+QF9TvE9a937RbzgTth71Lj65KN+5h/TDbdNppfmgIZbJPdeKpfB+bM2+j4j/zn+S4 wzsp816KuPHj5iHrGgOIx9CkNCm8nCqcxWKBJsyVpl4eiXH7HMVXDw5ilYJbKDnZMPTd XmQzpxFzPMANQQnS1K22mq96tGfGmJanatma0lOUp8cZvJLWZXsca5uuEKK1/fcNr9Xz paKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695517395; x=1696122195; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=xoPJblD6RR86hx7srJwcXnNzSH4ewmZToB9+JG9whdw=; b=ojn+pyIvVhUcfmfbdVt0TwT9AdroA3frLASpF0q/FrBlX0Mn20H0fa65o0f+qku5Jx yZEemZbqH0+kzkk9yCgyz5r7JrfJF+OMZkQpyQbYDkpQzGrvUJq6yX6LGOkQQiuD3z/w pE36lgna+i3f+U3NV08rcn7bL/azuXP2Sujao8gWXEgiMwU/cHrZmy4ReZllTmv31CQ+ TiOJjP/jvRgXSFEGYWnQoR2wqja3WOPJefR9YqbJoMAH0Sv23PhBOo82vVNGHlabwsHG XkB8aR+Mffwqkcf67W+KZ1fIsXBnglairlWj+nujjFq9dkShf4ZHpi+5So1wmiFHC8Bj 2PcQ== X-Gm-Message-State: AOJu0Yyxj8PVXpnlgeFPBL3qrgt1avwSyn5QmOoV2Jpe36CHXvbbm244 vC1+KzFyJFbGTgg559/UmTA/YQ0/Oii7UQ7GxBs= X-Google-Smtp-Source: AGHT+IE9FSlVAR+zhX9mfwEC49JUMo+iWfq985mrH6XkyLr/NjPNLf39WSCWn3CXlCA9amSExFQnDdYgyajnuUMJy1M= X-Received: by 2002:a5d:4c8c:0:b0:31a:e6c2:770d with SMTP id z12-20020a5d4c8c000000b0031ae6c2770dmr2903482wrs.36.1695517395039; Sat, 23 Sep 2023 18:03:15 -0700 (PDT) In-Reply-To: Received-SPF: pass client-ip=2a00:1450:4864:20::430; envelope-from=dinnyesd@gmail.com; helo=mail-wr1-x430.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org Original-Sender: guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.lisp.guile.devel:21971 Archived-At: --0000000000000ab93e0606106846 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 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 u= p, > > etc. > > > > I think it's in better shape now. Checked that tests run at least! > > It's the same feature branch URL > > ... 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. =E2=80=98use defaults for undefine= d > parameter objects=E2=80=99 in (guile)Expect. > > Likewise, the other commits need some changes in commit message. > Done > > 1. The first commit and =E2=80=98Added original attribution comments=E2= =80=99 > 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 =E2=80=98Added > original comments=E2=80=99. > Squashed them together, but the macros to which the doc-strings apply don't exist in the initial commit. > > 3. For =E2=80=98use 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: =E2= =80=98This > =E2=80=98Nowadays something like this would be implemented with parameter > objects or syntax-parameterize, but that would be a backwards > incompatible change.=E2=80=99. > 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 =E2=80=98Added interact macro implementation=E2=80=99, 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. > --=20 Best regards, Daniel Dinnyes --0000000000000ab93e0606106846 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Maxime,

= Apologies for the 3 month delay in the reply, but it took some time to unti= l 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 t= he earlier.

Please find my response to your point= s 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/tre= e/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= =C2=A0
Done =
1(b) There is already an expect module; the hygiene bit is the difference.<= br>
Done
1(c) There are some missing bits, e.g. =E2=80=98use defaults for undefined =
parameter objects=E2=80=99 in (guile)Expect.

Likewise, the other commits need some changes in commit message.
=C2=A0Done

1. The first commit and =E2=80=98Added original attribution comments=E2=80= =99
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 =E2=80=98Added =
original comments=E2=80=99.
=C2=A0
Squashed them together, but the macros to wh= ich the doc-strings apply don't exist in the initial commit.=C2=A0

3. For =E2=80=98use defaults for undefined parameter bindings': Guile h= as a
thing named 'parameters' (search for make-parameter etc.), which is= n't
what is used here.=C2=A0 I recommend searching for other terminology.

Also, for clarity, I would add to the commit message something like: =E2=80= =98This
=E2=80=98Nowadays something like this would be implemented with parameter <= br> objects or syntax-parameterize, but that would be a backwards
incompatible change.=E2=80=99.

This i= s where the new code has the great difference compared to the earlier imple= mentation.
I've rew= ritten the core expect-with-bindings macro, such that instead of juggling w= ith passing arguments around in that recursive state-machine macro, it now = uses these module-internal named parameters.
Regarding backwards compatibility passing these par= ameters via lexical binding takes priority over parameterizing named the na= med parameters.
To = ensure compatibility, the named parameter bindings are not exported, and co= uld be used only via module-internal references like in here.

4. In =E2=80=98Added interact macro implementation=E2=80=99, there is:

=C2=A0 > +(define interact-expect-port-error
=C2=A0 > +)

=C2=A0 =C2=A0 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.
=C2=A0

Best regards,
Maxime Devos.


--
Best regards,
Daniel Dinnyes
<= /div>
--0000000000000ab93e0606106846--