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

[Taiju HIGASHI]

Hi,

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

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

I have listed them all in the v5 patch.
As for the serializer/predicate procedure, I did not add it because
there was no docstring in the existing procedure.

>> +(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”.

I may have confused it with Common Lisp.  I have eliminated the field
with the empty list as the default value.

>> +(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”.

Fixed to xml-string.

>> +(define font-directories? list?)
>
> Is it really needed?

I may not have addressed this point yet. Is it possible to not define a
predicate procedure to be used for a configuration field?

>> +(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))

I think I fixed it by refactoring.

>> +   (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.

I have fixed it.

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

Since there were many points raised and interface changes in this case,
I will revise the document after the review is complete.

Thanks,
--
Taiju