[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#28185] [PATCH] build: emacs-build-system: Make the install phase mo
[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.
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?'.
> 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?'
> 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?
Ah, yes. Sorry, I'm confusing scheme's behaviour with that of other
lisps. We still need `null?'.
The patch LGTM.
|[Prev in Thread]
||[Next in Thread]|
- [bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.,
Arun Isaac <=