GNU bug report logs - #64620
[PATCH] gnu: home: Add home-emacs-service-type.

Previous Next

Full log

Message #59 received at 64620 <at> debbugs.gnu.org (full text, mbox):

From: Kierin Bell <fernseed <at> fernseed.me>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Tobias Geerinckx-Rice <me <at> tobias.gr>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, paren <at> disroot.org,
 Christopher Baines <mail <at> cbaines.net>, 64620 <at> debbugs.gnu.org,
 Andrew Tropin <andrew <at> trop.in>, Ricardo Wurmus <rekado <at> elephly.net>,
 Mathieu Othacehe <othacehe <at> gnu.org>
Subject: Re: [bug#64620] [PATCH] gnu: home: Add home-emacs-service-type.
Date: Thu, 12 Oct 2023 18:26:20 -0400

Ludovic Courtès <ludo <at> gnu.org> writes:

> So, here is a short review of the parts I’m most familiar with.
>
> fernseed <at> fernseed.me skribis:
>
>
> [...]
>
>> +(define-module (gnu home services emacs)
>> +  #:use-module (gnu home services)
>
> [...]
>
>> +  #:re-export (blank?
>> +
>> +               vertical-space
>> +               vertical-space?
>
> Why re-export these things from here?  Sounds surprising because we’re
> in a service module.
>
>

IIRC, my rationale was that the `<elisp>' objects can contain <blank>
records, e.g., `elisp->sexp' can return them.  But users won't normally
be manipulating them, so I should probably remove this.

>
> For clarity/conciseness, you can use #~ and #$ in this code.
>

Thanks, fixed.

>
> I think you don’t need to fiddle with the monadic interface.  I’d
> suggest removing the <elisp-file> type and gexp compiler and instead
> defining ‘elisp-file’ in terms of ‘computed-file’.  WDYT?
>

This would prevent people from being able to reference Elisp files in
G-expressions, or from writing something like:

--8<---------------cut here---------------start------------->8---
(elisp-file "elisp-file-with-elisp-file"
            (list (elisp (load (unelisp %another-elisp-file)))))
--8<---------------cut here---------------end--------------->8---

...Right?

As long as the Emacs home service type offers a better way to load
files, which I think it does, I'm OK with losing this capability.

>> +(define (record-value rec field)
>> +  "Return the value of field named FIELD in record REC."
>> +  ((record-accessor (record-type-descriptor rec) field) rec))
>> +
>> +(define-syntax extend-record
>> +  ;; Extend record ORIGINAL by creating a new copy using CONSTRUCTOR,
>> +  ;; replacing each field specified by ORIG-FIELD with the evaluation of (PROC
>> +  ;; ORIG-VAL EXT-VALS), where ORIG-VAL is the value of ORIG-FIELD in ORIGINAL
>> +  ;; and EXT-VALS is the list of values of EXT-FIELD in EXTENSIONS.
>> +  (lambda (s)
>> +    (syntax-case s ()
>> +      ((_ constructor original extensions (proc orig-field ext-field) ...)
>> +       (with-syntax (((field-specs ...)
>> +                      (map
>> +                       (lambda (spec)
>> +                         (syntax-case spec ()
>> +                           ((proc orig-field ext-field)
>> +                            #'(orig-field
>> +                               (apply
>> +                                proc
>> +                                (list
>> +                                 (record-value original 'orig-field)
>
> I would advice against accessing record fields by name, with run-time
> field name resolution.
>
> The spirit of records, unlike alists, is that there’s a
> statically-defined mapping of fields to their offsets in the struct;
> without having access to record accessors, you’re not supposed to be
> able to access the record (I know Guile has ‘struct-ref’,
> ‘record-accessor’, etc., but these are abstraction-breaking primitives
> that should be avoided IMO).
>
> How could this code be adjusted accordingly?  I guess you’re looking for
> a way to iterate over fields?
>

From what I remember, the alternatives are all complicated.

I do find `extend-record' to be very useful, especially when services
need to extend configuration records that have nested records.  It's
useful enough that I'd like to see it exported from somewhere, but `(gnu
home services emacs)' hardly seems like the place.

I propose that we put `extend-record' and a few of the most useful
`extend-record-*' procedures --- like `extend-alist-merge' and
`extend-record-field-default' (which I rewrote since v1 of the patch
because there was a bug) --- into `(guix records)'.

There, it could use `lookup-field+wrapper' to manually find the offset
like `match-record'.  This isn't exactly ideal, as it would still need
to then use `struct-ref' or similar, but at least `match-record' does
this, too.

WDYT?

>> +;;; Elisp reader extension.
>> +;;;
>> +
>> +(eval-when (expand load eval)
>> +
>> +  (define (read-elisp-extended port)
>> +    (read-with-comments port
>> +                        #:blank-line? #f
>> +                        #:elisp? #t
>> +                        #:unelisp-extensions? #t))
>> +
>> +  (define (read-elisp-expression chr port)
>> +    `(elisp ,(read-elisp-extended port)))
>> +
>> +  (read-hash-extend #\% read-elisp-expression))
>
> I’d lean towards not having a reader extension because they don’t
> compose and it’s easy to end up colliding with another, unrelated
> extension.  I think it’s okay if people write:
>
>   (elisp …)
>
> rather than:
>
>   #%(…)
>
> It’s also arguably easier to understand for a newcomer.
>

I'm OK with losing the reader extension.  After some experience, I find
that I'd rather use `elisp'.  Being able to use semicolons for comments
via reader extension is impressive but also weirdly unsettling.

Also, anyone who wants to seriously write a lot of Elisp-in-Scheme will
want to instead use the `elisp*' macro, which can contain multiple
s-exps.  And in order to use the reader extension there, you'd need to
write something like:

(elisp*
  ;; ...
  (unelisp #%SEXP-1)
  (unelisp #%SEXP-2)
  ;; ...
  )

>> +++ b/guix/read-print.scm
>
> This part is the most “problematic” for me: I’m already dissatisfied
> with the current state of things (the pretty-printer in particular is
> too complex and hard to work with), and this change brings more
> complexity and lacks orthogonality.
>
> What I’d like to see, ideally, is a clear separation between elisp
> concerns and Scheme concerns in the reader and in the pretty printer.
>
> Probably, a preliminary step (I could look into it) would be to rewrite
> the pretty printer based on Wadler’s “prettier printer” paper and/or
> Shinn’s formatting combinators¹.
>
> WDYT?
>

I can honestly say that I'm dreading splitting the `(guix read-print)'
changes I've made into multiple commits and annotating them properly.
Writing the pretty printer from scratch sounds a lot less confusing and
painful at this point.

I'm imagining a stripped-down version of the "formatting combinators"
from SRFI-159/166 specifically adapted for pulling into service modules
to write pretty-printers, not just for Elisp but also for other
configuration languages.

It's too bad that Guile doesn't have SRFI-159/166 and the requisite
"environment monads" and delayed computation from SRFI-165 built-in.

My first design question would be: Would this be a suitable application
for `(guix monads)' [to create a formatting "environment monad"], or
does that entail more trouble than it's worth?

I'll work on the unit tests, as well, especially once more progress has
been made on the pretty-printer situation.

> Thanks,
> Ludo’.

Thanks, I appreciate the feedback!

-- 
Kierin Bell
GPG Key: FCF2 5F08 EA4F 2E3D C7C3  0D41 D14A 8CD3 2D97 0B36

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.