Hi, Ludo, Thanks for comments!
l...@gnu.org (Ludovic Courtès) writes: > Huang Ying <huang.ying.cari...@gmail.com> skribis: > >> * guix/profiles.scm (fonts-dir-file): Create fonts.dir/scale files for all >> fonts directories. > > Looks cool, modulo Danny’s suggestions and minor issues below. > > BTW, could you explain (probably as a comment in the code) why we need > to do all this? What is it fixing? :-) Sure. >> + (union-build fonts-dir fonts-dirs >> + #:log-port (%make-void-port "w") >> + #:create-all-directories? #t) >> + (ftw fonts-dir >> + (lambda (dir statinfo flag) > > ‘ftw’ is not great IMO and not used in Guix. > > I would suggest using ‘find-files’ from (guix build utils). You could > write something like: > > (let ((directories (find-files fonts-dir > (lambda (file stat) > (eq? 'directory (stat:type stat))) > #:directories? #t))) > (for-each do-something directories)) > > This also has the advantage of separating the generation of the > directory list from the actual action. Sure. >> + (and (eq? flag 'directory) >> + (with-directory-excursion dir >> + (and (file-exists? fonts-scale-file) >> + (delete-file fonts-scale-file)) > > Here, since ‘delete-file’ has a side-effect and a meaningless return > value, we conventionally avoid ‘and’ and instead write: > > (when (file-exists? fonts-scale-file) > (delete-file fonts-scale-file)) > > for clarity. Sure >> + (and (file-exists? fonts-dir-file) >> + (delete-file fonts-dir-file)) >> + (system* mkfontscale) >> + (system* mkfontdir) >> + (and (empty-file? fonts-scale-file) >> + (delete-file fonts-scale-file)) >> + (and (empty-file? fonts-dir-file) >> + (delete-file fonts-dir-file)))) > > Same as above. Sure. Best Regards, Huang, Ying > Thank you! > > Ludo’.
