On Wed, 30 Aug 2017 13:37:49 +0530 Arun Isaac wrote: > Ok. Ludo has spoken. :-) Let us proceed with the patch review. > > > - (define (install-file? file stat) > > - (let ((stripped-file (string-trim (string-drop file > > (string-length source)) #\/))) > > - (and (any (cut string-match <> stripped-file) include) > > - (not (any (cut string-match <> stripped-file) > > exclude))))) > > + (define* (install-file? file stat #:key verbose?) > > + (let* ((stripped-file (string-trim > > + (string-drop file (string-length > > source)) #\/))) > > + (define (match-stripped-file action regex) > > + (let ((result (string-match regex stripped-file))) > > + (if (and result verbose?) > > + (format #t "info: ~A ~A as it matches \"~A\"\n" > > + stripped-file action regex)) > > + result)) > > + > > + (if verbose? > > + (format #t "info: considering installing ~A\n" > > stripped-file)) + > > + (and (any (cut match-stripped-file "included" <>) include) > > + (not (any (cut match-stripped-file "excluded" <>) > > exclude))))) > > If I understand this correctly, `install-file?' will be called with > `#:verbose #t' only when no files were installed. In that case, we > would simply get a long list of "excluded" statements. This seems a > bit redundant. Do we really need this? Here is a real example of what the output can look like: error: No files found to install. info: considering installing .gitignore info: considering installing .travis.yml info: considering installing Cask info: considering installing README.md info: considering installing Rakefile info: considering installing minitest.el info: minitest.el included as it matches "^[^/]*\.el$" info: minitest.el excluded as it matches "^[^/]*tests?\.el$" info: considering installing tools/snippet_extractor.rb info: considering installing test/minitest-unit-test.el info: considering installing test/test-helper.el info: considering installing snippets/minitest-mode/assert info: considering installing snippets/minitest-mode/assert_empty info: considering installing snippets/minitest-mode/assert_equal info: considering installing snippets/minitest-mode/assert_in_delta info: considering installing snippets/minitest-mode/assert_in_epsilon info: considering installing snippets/minitest-mode/assert_includes info: considering installing snippets/minitest-mode/assert_instance_of info: considering installing snippets/minitest-mode/assert_kind_of info: considering installing snippets/minitest-mode/assert_match info: considering installing snippets/minitest-mode/assert_nil info: considering installing snippets/minitest-mode/assert_operator info: considering installing snippets/minitest-mode/assert_output info: considering installing snippets/minitest-mode/assert_predicate info: considering installing snippets/minitest-mode/assert_raises info: considering installing snippets/minitest-mode/assert_respond_to info: considering installing snippets/minitest-mode/assert_same info: considering installing snippets/minitest-mode/assert_send info: considering installing snippets/minitest-mode/assert_silent info: considering installing snippets/minitest-mode/assert_throws info: considering installing snippets/minitest-mode/flunk info: considering installing snippets/minitest-mode/pass info: considering installing snippets/minitest-mode/refute info: considering installing snippets/minitest-mode/refute_empty info: considering installing snippets/minitest-mode/refute_equal info: considering installing snippets/minitest-mode/refute_in_delta info: considering installing snippets/minitest-mode/refute_in_epsilon info: considering installing snippets/minitest-mode/refute_includes info: considering installing snippets/minitest-mode/refute_instance_of info: considering installing snippets/minitest-mode/refute_kind_of info: considering installing snippets/minitest-mode/refute_match info: considering installing snippets/minitest-mode/refute_nil info: considering installing snippets/minitest-mode/refute_operator info: considering installing snippets/minitest-mode/refute_predicate info: considering installing snippets/minitest-mode/refute_respond_to info: considering installing snippets/minitest-mode/refute_same info: considering installing snippets/minitest-mode/skip > ... continued below > > > + (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? > > + #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))?