Fixes issue #555

[DonyorM]

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.

[DonyorM]

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]

@arichiardi Updated it to match your suggestions.

[arichiardi]

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]

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]

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

[DonyorM]

Soooo, what do we want to do with this?

[arichiardi]

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]

I was partially seeking those other opinions :)

[martinklepsch]

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]

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]

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

[DonyorM]

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

[arichiardi]

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

[DonyorM]

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

[arichiardi]

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]

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

[DonyorM]

Anything else I need to do for this?

[arichiardi]

No :+1: I think @alandipert or @micha will merge it eventually.

[martinklepsch]

👍