Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

argument $masks in find methods made required #5

Closed
wants to merge 1 commit into from

Conversation

@vlastavesely
Copy link

vlastavesely commented Feb 12, 2017

  • bug fix? yes
  • BC break? no
@dg

This comment has been minimized.

Copy link
Member

dg commented Feb 19, 2017

I like more explicit findFiles('*')

@vlastavesely

This comment has been minimized.

Copy link
Author

vlastavesely commented Feb 20, 2017

OK, but, in my opinion, it is problem that if you call it with empty $masks, you get notice Undefined offset: 0 in instead of something more meaningful.

@dg

This comment has been minimized.

Copy link
Member

dg commented Feb 20, 2017

And can you fix it?

@vlastavesely

This comment has been minimized.

Copy link
Author

vlastavesely commented Feb 20, 2017

yes

@vlastavesely vlastavesely force-pushed the vlastavesely:master branch from 9417084 to 8fa3c46 Feb 20, 2017
@vlastavesely

This comment has been minimized.

Copy link
Author

vlastavesely commented Feb 20, 2017

Argument $masks is now required. Do you like it more?

@vlastavesely vlastavesely changed the title added fallback for empty $masks in find methods argument $masks in find methods made required Feb 20, 2017
@dg

This comment has been minimized.

Copy link
Member

dg commented Feb 20, 2017

What about to change declaration to findFiles($mask, ...$masks), ie. native enforce first parameter?

@dg

This comment has been minimized.

Copy link
Member

dg commented Feb 20, 2017

And perhaps use only one way: the array or more parameters. The second way should be deprecated.

@vlastavesely

This comment has been minimized.

Copy link
Author

vlastavesely commented Feb 20, 2017

What about to change declaration to findFiles($mask, ...$masks), ie. native enforce first parameter?

I think it would be unnecessarily overcombined and little bit confusing.

@vlastavesely

This comment has been minimized.

Copy link
Author

vlastavesely commented Mar 7, 2017

What about to unify API of Finder and RobotLoader in some way? RobotLoader accepts array or comma separated string while Finder accepts array or argument list. I like the array|string version or only array eventually.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Mar 7, 2017

It may be just me, but I would prefer that calling findFiles() without arguments was allowed and always resulted in finding nothing.

@vlastavesely

This comment has been minimized.

Copy link
Author

vlastavesely commented Mar 7, 2017

I can imagine I would be a little bit confused if function named find() returned nothing and did not throw any warning or error.

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Mar 7, 2017

and always resulted in finding nothing

And someone might expect the opposite, find all files (i.e. no mask -> no filter -> find everything).

@f3l1x

This comment has been minimized.

Copy link
Member

f3l1x commented Jun 21, 2017

I agree with @Majkl578 and also with @JanTvrdik.

findFiles can be understood as find-all as well as find-nothing. What about clarify to findFiles()->all(),allFiles, findFiles(Finder::ALL). Also we could require at least one argument to findFiles.

Just an idea, let's discuss it.

@vlastavesely

This comment has been minimized.

Copy link
Author

vlastavesely commented Jun 22, 2017

According to me, function that does not find anything by default is useless.

Take Linux command find for example. Does not matter whether you run it by find or find .. It always finds all files in the directory unless you limit it by some pattern.

Another thing: imagine you have a directory and you want to check whether it is empty. You call Finder::findFiles(), it finds nothing and you remove the directory with confidence. But it contained important files you did not back up... You lost your files only because you forget to specify pattern when finding files...

@enumag

This comment has been minimized.

Copy link

enumag commented Jun 22, 2017

I also think findFiles without any mask should return all files. It makes more sense then returning nothing and is consistent with similar tools.

@dg dg closed this in 4958820 Jun 22, 2017
dg added a commit that referenced this pull request Jun 22, 2017
@f3l1x

This comment has been minimized.

Copy link
Member

f3l1x commented Jun 22, 2017

Thanks @dg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.