On Fri, 01 Sep 2017 10:32:18 +0530
Arun Isaac <arunisaac@systemreboot.net> wrote:

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

Great, I've pushed this now :)