guix-patches
[Top][All Lists]
Advanced

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

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase mo


From: Arun Isaac
Subject: [bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.
Date: Fri, 01 Sep 2017 10:32:18 +0530

>> > +    (if (null? files-to-install)
>> > +        (begin
>> > +          (format #t "error: No files found to install.\n")
>> > +          (find-files source (lambda (file stat)
>> > +                               (install-file? file stat #:verbose?
>> > #t)))
>>
>> I understand that the verbose output also shows the regexp due to
>> which the file was excluded, and that has debugging value. But,
>> perhaps, it'll be better to just print that here instead of a call
>> back to `install-file?'.
>>
>> WDYT?
>
> I put this in install-file? as I didn't want to duplicate the behaviour
> inside the function, firstly to avoid duplication, but also to avoid
> the possiblily that if they were duplicated, they would diverge in
> behaviour. I'm happy to experiment with splitting the "verbose" output
> out of install-file though?

Yes, I meant that you should split the verbose output out of
`install-file?'. But, it's not a big deal. I'm only nitpicking. Do go
ahead with your original code if you think it's alright.

>> > +          #f)
>> > +        (begin
>> > +          (for-each
>> > +           (lambda (file)
>> > +             (let* ((stripped-file (string-drop file
>> > (string-length source)))
>> > +                    (target-file (string-append target-directory
>> > stripped-file)))
>> > +               (format #t "`~a' -> `~a'~%" file target-file)
>> > +               (install-file file (dirname target-file))))
>> > +           files-to-install)
>> > +          #t))))
>>
>> Could you please rewrite this section using `cond' instead of `if'?
>> That way, we can get rid of the `begin'. Also, it might be better if
>> we used "positive logic" -- meaning, we first check for truthiness of
>> `files-to-install' instead of checking for (null?
>> files-to-install). Then, the else statement would take care of the
>> empty files-to-install case, and we would never have to call `null?'
>> explicitly.
>
> I tried this, but it seems that cond will treat '() as if it evaluates
> to true:
>
> scheme@(guile-user)> (cond ('() #t) (else #f))
> $1 = #t
>
> So this might need to still use null?, or even (not (null?
> files-to-install))?

Ah, yes. Sorry, I'm confusing scheme's behaviour with that of other
lisps. We still need `null?'.

The patch LGTM.





reply via email to

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