guix-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[bug#57963] [PATCH v4 2/2] home: fontutils: Support user's fontconfig.


From: Ludovic Courtès
Subject: [bug#57963] [PATCH v4 2/2] home: fontutils: Support user's fontconfig.
Date: Sat, 01 Oct 2022 23:57:18 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/28.1 (gnu/linux)

Taiju HIGASHI <higashi@taiju.info> skribis:

> * gnu/home/services/fontutils.scm: Support user's fontconfig.

I’m nitpicking a bit, but here we would describe all the
variables/procedures added, removed, or modified.  Please check ‘git
log’ for examples.

Regarding code, there’s a convention to add a docstring to each
top-level procedure:

  https://guix.gnu.org/manual/devel/en/html_node/Formatting-Code.html

It would be nice to follow it here.

> +(define (default-font-sanitizer type)
> +  (lambda (value)
> +    (if (null? value)
> +        value
> +        `(alias
> +          (family ,type)
> +          (prefer
> +           (family ,value))))))

Giving '() special meaning here looks quite unusual.  As Liliana wrote,
we’d usually use #f as the value denoting “nothing”.

> +(define (sxml->xmlstring sxml)
> +  (if (null? sxml)
> +      ""
> +      (call-with-output-string
> +        (lambda (port)
> +          (sxml->xml sxml port)))))

Same here.  Also, “xml-string” rather than “xmlstring”.

> +(define font-directories? list?)

Is it really needed?

> +(define (serialize-font-directories field-name value)
> +  (sxml->xmlstring
> +   (append
> +       '((dir "~/.guix-home/profile/share/fonts"))
> +       (map
> +        (lambda (path)
> +          `(dir ,path))
> +        value))))

The indentation would rather be:

  (append '((dir …))
          (map (lambda (directory)
                 `(dir ,directory))
               value))

> +   (map (match-lambda
> +          ((? pair? sxml) sxml)
> +          ((? string? xml) (xml->sxml xml))
> +          (_ (error "extra-config value must be xml string or sxml list.")))

Instead of ‘error’, which would lead to an ugly backtrace and an
untranslated error message, write:

  (raise (formatted-message (G_ "'extra-config' …")))

without a trailing dot in the message.

The rest LGTM!  Like I wrote, could you please add documentation in
‘doc/guix.texi’, with a configuration example like the one you gave?

Thanks for all the work!

Ludo’.





reply via email to

[Prev in Thread] Current Thread [Next in Thread]