Andrew Tropin writes: > On 2022-10-11 12:54, Taiju HIGASHI wrote: > >> Hi Andrew, >> >> Thank you for your review! >> >>>> +(define (string-list? value) >>>> + (and (pair? value) (every string? value))) >>> >>> Better to use list? here and in the other places, at least for the >>> consistency, but also for semantic meaning. >> >> OK. I'll rewrite it to below. >> >> --8<---------------cut here---------------start------------->8--- >> (define (string-list? value) >> (and (list? value) (every string? value))) >> --8<---------------cut here---------------end--------------->8--- >> >>>> + >>>> +(define (serialize-string-list field-name value) >>>> + (sxml->xml-string >>>> + (map >>>> + (lambda (path) `(dir ,path)) >>>> + (if (member guix-home-font-dir value) >>>> + value >>>> + (append (list guix-home-font-dir) value))))) >>>> + >>>> +(define (serialize-string field-name value) >>>> + (define (serialize type value) >>>> + (sxml->xml-string >>>> + `(alias >>>> + (family ,type) >>>> + (prefer >>>> + (family ,value))))) >>>> + (match (list field-name value) >>>> + (('default-font-serif-family family) >>>> + (serialize 'serif family)) >>>> + (('default-font-sans-serif-family family) >>>> + (serialize 'sans-serif family)) >>>> + (('default-font-monospace-family family) >>>> + (serialize 'monospace family)))) >>>> + >>>> +(define-maybe string) >>>> + >>>> +(define extra-config-list? list?) >>>> + >>>> +(define-maybe extra-config-list) >>>> + >>>> +(define (serialize-extra-config-list field-name value) >>>> + (sxml->xml-string >>>> + (map (match-lambda >>>> + ((? pair? sxml) sxml) >>> >>> Other branches would never be visited because it will fail earlier by >>> define-configuration predicate check for extra-config-list? (which is >>> basically list?). > > Oh, I missed the map over the list elements and slightly missread the > code. I thought (according to my incorrect perception of > implementation) extra-config have to be either sxml or string, that's is > why I said that it will fail earlier because plan string value won't > satisfy list predicate attached to extra-config field, but in a fact > extra-config is always a list, but can be a list of sxml's and strings > mixed together. > > Thus, some of my comments are invalid. Sorry for the confusion. I'll > rephrase and elaborate in the later message. I was worried that I was the only one who did not understand the code I wrote, but I've relieved to hear that it was a misunderstanding :) Is it OK to have multiple data types (XML string and SXML list) in a list? >> We can specify invalid value such as (list "foo" '(foo bar) 123). >> >>> Also, making multi-type fields is debatable, but isn't great IMO. >> >> I see. If we had to choose one or the other, I would prefer the >> string-type field. >> >>> If serialization would support G-exps, we could write >>> >>> (list #~"RAW_XML_HERE") >>> >>> or even something like this: >>> >>> (list #~(READ-THE-WHOLE-FILE #$(local-file "our-old.xml"))) >> >> Does it mean that the specification does not allow it now? Or does it >> mean that it is not possible with my implementation? >> > > It's not possible with the current implementation. I'll try to modify it so that it can support G-exps. >>>> + ((? string? xml) (xml->sxml xml)) + (else + (raise >>>> (formatted-message + (G_ "'extra-config' type must be xml string or >>>> sxml list, was given: ~a") + value)))) + value))) + >>>> +(define-configuration home-fontconfig-configuration + >>>> (font-directories + (string-list (list guix-home-font-dir)) >>> >>> It's not a generic string-list, but a specific font-directories-list >>> with extra logic inside. >>> >>> Also, because guix-home-font-dir always added to the list, the default >>> value should '() and field should be called additional-font-directories >>> instead. Otherwise it will be confusing, why guix-home-font-dir is not >>> removed from the final configuration, when this field is set to a >>> different value. >>> >>> I skimmed previous messages, but sorry, if I missed any already >>> mentioned points. >> >> Sure, It is more appropriate that the field type is to >> font-directories-list field name is to additional-font-directories. >> > > As Liliana mentioned in the other message, it's better not to set > anything implicitly. It's better to keep the name, but change the > implementation and remove implicitly and unconditionally added > directory. OK. I'll modify the default value to an empty list and include ~/.guix-home/profile/share/fonts in the sample code in the documentation. >>>> + "The directory list that provides fonts.") >>>> + (default-font-serif-family >>>> + maybe-string >>>> + "The preffered default fonts of serif.") >>>> + (default-font-sans-serif-family >>>> + maybe-string >>>> + "The preffered default fonts of sans-serif.") >>>> + (default-font-monospace-family >>>> + maybe-string >>>> + "The preffered default fonts of monospace.") >>>> + (extra-config >>>> + maybe-extra-config-list >>>> + "Extra configuration values to append to the fonts.conf.")) >>>> + >>>> +(define (add-fontconfig-config-file user-config) >>>> `(("fontconfig/fonts.conf" >>>> ,(mixed-text-file >>>> "fonts.conf" >>>> " >>>> >>>> - >>>> - ~/.guix-home/profile/share/fonts >>>> -")))) >>>> +" >>>> + (serialize-configuration user-config home-fontconfig-configuration-fields) >>> >>> Just a thought for the future and a point for configuration module >>> improvements: It would be cool if serialize-configuration and all other >>> serialize- functions returned a G-exps, this way we could write >>> something like that: >>> >>> (home-fontconfig-configuration >>> (font-directories (list (file-append font-iosevka "/share/fonts")))) >> >> Nice. >> >> Thanks, >> -- >> Taiju Thanks, -- Taiju