Fixes issue #555 #608

DonyorM · 2017-05-01T21:22:38Z

This fixes issue #555

This modifies the file-filter function so that if the criteria seq is
empty, it simply returns an empty list.

An empty list was chosen instead of simply returning nil because an empty list allows seq functions (conj, cons, etc.) to continue to work.

arichiardi · 2017-05-01T21:26:03Z

boot/core/src/boot/core.clj

-          ->file #(if (tmp? %) (io/file (tmp-path %)) (io/file %))
-          pred (apply juxt (mapv mkpred criteria))]
-      (filter-files #(pred (->file %)) files negate?))))
+    (if (empty? criteria)


this works, but a more idiomatic way would be:

(when (seq criteria)) ....

Would that not result in a nil return if the seq was empty? I'm still new to Clojure and Boot, so I may be missing something.

Yep it returns nil.
I have been thought that you should always check using (seq coll) if the collection contains stuff (wonderfully in The Joy of Clojure). This has to do with how lazy sequences where implemented in Clojure and it is very different from how things are done in other Lisps (you can google nil punning and nil punning next rest in Clojure).

DonyorM · 2017-05-01T21:26:20Z

Huh, I just noticed that #600 fixed this issue as well. hit023 said for me to do ahead and fix it, and I hadn't thought about checking pull requests.

Anyway, this does have the advantage of passing the tests.

DonyorM · 2017-05-02T15:39:15Z

@arichiardi Updated it to match your suggestions.

arichiardi · 2017-05-02T15:46:11Z

You know I have been thinking about this and I was wondering whether it is a good idea to filter all the items if no criterias are given...Maybe this is why the behavior was to crash..Is nil the same as passing an identity function? Does it need to flip behavior when negate? is true?

The least surprising thing is maybe to return all the files in case of empty criteria, maybe, thoughts?

DonyorM · 2017-05-02T22:41:14Z

Though I assume you were asking that as a more general, question, I'll still put my thoughts down.

This is basically a divide by zero situation. Does nothing match anything?

More concretely, my suggest would to be return nil following from the divide by zero situation. If you do something impossible, return a result that's basically "undefined." (Ok the analogy is a stretch).

arichiardi · 2017-05-03T00:17:02Z

But ok, so if the behavior is undefined, maybe it is better to throw the error then, this is my biggest doubt.

DonyorM · 2017-05-15T13:52:40Z

Soooo, what do we want to do with this?

arichiardi · 2017-05-15T14:48:27Z

Seeking advice? 😀 The undefined behavior above that you describe is good, but I still think nil carries some other connotation. Dunno maybe it is just me, I'd like to hear more opinions.

DonyorM · 2017-05-15T15:59:24Z

I was partially seeking those other opinions :)

martinklepsch · 2017-06-02T02:09:28Z

How about

(assert (seq criteria) "by-path requires a list of matching paths, if you want all files use boot.core/ls")

Or something like that. Perhaps added to all the by-* functions individually. It seems that passing no critera should essentially be undefined behavior, especially in combination with :negate? it can become weird.

What do you think?

arichiardi · 2017-06-02T02:57:38Z

I came to like the idea of an error actually there, so an assertion makes total sense to me. To all functions in order to specify what's missing. Totally. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

arichiardi · 2017-06-16T15:55:25Z

@DonyorM what do you think? Shall we finish this off? 😁

DonyorM · 2017-06-16T16:03:22Z

@arichiardi @martinklepsch Updated the commit. Let me know if I should ask anything else.

arichiardi · 2017-06-16T16:59:10Z

boot/core/src/boot/core.clj

@@ -1218,6 +1218,7 @@
  "A file filtering function factory. FIXME: more documenting here."
  [mkpred]
  (fn [criteria files & [negate?]]
+    (assert (seq criteria) "file-filet (called from by-*) requires a list of matching paths, if you want all files use boot.core/ls")


There is a lil' typo there and I would drop the if you want ... because if you call it from by-ext is a weird message back...my proposal:

boot.core/file-filter requires a list of criteria but null was passed in, make sure your `by-*` calls are passing them.

Looks good to me. Just pushed a new commit.

Still don't see it, GitHub seems a bit slow today

Whoops, sorry, somehow didn't commit the changes.

arichiardi · 2017-06-17T00:09:45Z

A part for Changelog and better working ( I am using to pass twice there lol) I think this is finally good to go 😀

DonyorM · 2017-06-17T14:08:33Z

@arichiardi Added changes.md. I put this under improved, since it's kind of new behavior

arichiardi · 2017-06-17T16:26:05Z

Thanks a lot @DonyorM and sorry for this long conversation, I maybe swayed you a bit from the final solution but I see this is an improvement. I spent myself quite some time trying to figure out what the failure was and opened the issue in frustration 😁😁 this will avoid the trouble for other folks 👍 Good job!

DonyorM · 2017-06-17T16:45:09Z

@arichiardi No problem. It wasn't simple to figure out the best way to do this. Thanks for your help!

DonyorM · 2017-07-09T14:54:25Z

Anything else I need to do for this?

arichiardi · 2017-07-09T15:37:12Z

No 👍 I think @alandipert or @micha will merge it eventually.

martinklepsch · 2017-07-09T15:40:45Z

👍

martinklepsch · 2017-07-09T15:41:25Z

boot/core/src/boot/core.clj

@@ -1218,6 +1218,7 @@
  "A file filtering function factory. FIXME: more documenting here."
  [mkpred]
  (fn [criteria files & [negate?]]
+    (assert (seq criteria) "boot.core/file-filter requires a list of criteria but null was passed in, make sure your `by-*` calls are passing them.")


actually null could be nil here but not too important haha :)

haha, I was stuck in java land there. Just pushed an update to change it.

This modifies the file-filter function so that if the criteria seq is empty, it throws an error.

arichiardi reviewed May 1, 2017

View reviewed changes

DonyorM force-pushed the fix-emptyPathError-555 branch from 1b4bed0 to 601cd23 Compare May 2, 2017 15:38

arichiardi mentioned this pull request May 27, 2017

Fixes #555 and #596 #600

Closed

DonyorM force-pushed the fix-emptyPathError-555 branch from 601cd23 to 63edf19 Compare June 16, 2017 16:02

DonyorM force-pushed the fix-emptyPathError-555 branch from 63edf19 to 4a51c1c Compare June 16, 2017 16:08

arichiardi reviewed Jun 16, 2017

View reviewed changes

DonyorM force-pushed the fix-emptyPathError-555 branch 2 times, most recently from de783e4 to 9d8446f Compare June 16, 2017 21:33

arichiardi added the Changelog missing label Jun 17, 2017

DonyorM force-pushed the fix-emptyPathError-555 branch from 9d8446f to f9213f1 Compare June 17, 2017 14:07

arichiardi added ready and removed Changelog missing labels Jun 17, 2017

martinklepsch approved these changes Jul 9, 2017

View reviewed changes

Fixes issue boot-clj#555

c3046ac

This modifies the file-filter function so that if the criteria seq is empty, it throws an error.

DonyorM force-pushed the fix-emptyPathError-555 branch from f9213f1 to c3046ac Compare July 11, 2017 14:22

alandipert merged commit cb923ee into boot-clj:master Jul 11, 2017

alandipert added this to the next-release milestone Jul 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes issue #555 #608

Fixes issue #555 #608

DonyorM commented May 1, 2017

arichiardi May 1, 2017

DonyorM May 1, 2017

arichiardi May 1, 2017

DonyorM commented May 1, 2017

DonyorM commented May 2, 2017

arichiardi commented May 2, 2017

DonyorM commented May 2, 2017

arichiardi commented May 3, 2017

DonyorM commented May 15, 2017

arichiardi commented May 15, 2017

DonyorM commented May 15, 2017

martinklepsch commented Jun 2, 2017

arichiardi commented Jun 2, 2017 via email

arichiardi commented Jun 16, 2017

DonyorM commented Jun 16, 2017

arichiardi Jun 16, 2017

DonyorM Jun 16, 2017

arichiardi Jun 16, 2017

DonyorM Jun 16, 2017

arichiardi commented Jun 17, 2017

DonyorM commented Jun 17, 2017

arichiardi commented Jun 17, 2017

DonyorM commented Jun 17, 2017

DonyorM commented Jul 9, 2017

arichiardi commented Jul 9, 2017

martinklepsch commented Jul 9, 2017

martinklepsch Jul 9, 2017

DonyorM Jul 11, 2017

Fixes issue #555 #608

Fixes issue #555 #608

Conversation

DonyorM commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonyorM commented May 1, 2017

DonyorM commented May 2, 2017

arichiardi commented May 2, 2017

DonyorM commented May 2, 2017

arichiardi commented May 3, 2017

DonyorM commented May 15, 2017

arichiardi commented May 15, 2017

DonyorM commented May 15, 2017

martinklepsch commented Jun 2, 2017

arichiardi commented Jun 2, 2017 via email

arichiardi commented Jun 16, 2017

DonyorM commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichiardi commented Jun 17, 2017

DonyorM commented Jun 17, 2017

arichiardi commented Jun 17, 2017

DonyorM commented Jun 17, 2017

DonyorM commented Jul 9, 2017

arichiardi commented Jul 9, 2017

martinklepsch commented Jul 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment