opened
50bhan
username_0
| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| Fixed tickets | #355 

This PR will close #355 for support of multiple files/folders on input.

There is couple of thing that should be considered:
- First, some parts of this PR is based on @username_3 awesome PR #382. So, I assumed his PR will be merged first.
- Secondly, there are some discussable points in this code, so we should discuss about them.
Feat/support multiple path
issue
created
nunomaduro
username_1
I think we should target the 2.x version.
comment
created
50bhan
username_0
@username_1 If it's the case, i'm cool with that! Maybe we'll be able to add some related features to it as well (i.e. #354)
comment
created
olivernybroe
username_2
@username_0 Adding support for this, will make it a lot easier to add support for #354. However that should be in a pr for itself :)
comment
created
50bhan
username_0
@username_2 Yeah sure it is. Honestly I hate waiting too much for tests to pass or FAIL :laughing: So, I took this task so #354 will be possible after it. And of course in a separate PR.
comment
created
50bhan
username_0
@username_2 Sorry to bother, I did those changes and I will be appreciate if you take a look at it. Thanks.
comment
created
50bhan
username_0
@username_2 Thank you for fixing the bug and test. I imagine there is nothing more for this task. Let me know if something came up regarding this PR.
comment
created
olivernybroe
username_2
@username_0 Just merged jibbarths speed improvements, can you resolve the conflicts quickly? 👍
comment
created
50bhan
username_0
@username_2 Yeah i'm on it.
comment
created
olivernybroe
username_2
Hey @username_3 can I get your to make a review of this? 

I have tested it locally and it works 😄
comment
created
Jibbarth
username_3
@username_2 Sure, I'll check this tonight :+1:
comment
created
50bhan
username_0
@username_3 Welcome :wink: Thanks for the review! Yeah I totally forgot about docs. I will add it and go for your comments.
comment
created
olivernybroe
username_2
@username_0 I found a bug when adding in a file which also exist in the vendor folder. 
So for example running `bin/phpinsights analyse CHANGELOG.md` results in 260 files found, because it finds a bunch in the vendor folder.
comment
created
50bhan
username_0
@username_2 I'm tuning the code a little bit, after that I will fix it as well.
comment
created
olivernybroe
username_2
@username_0 Cool! 
Just a quick example of something we can do (might need some fine-tuning) to only run on changed files when this is merged in
```bash
bin/phpinsights analyse $(git --no-pager diff master HEAD --name-only -- '*.php' | tr '\n' ' ')
```
comment
created
50bhan
username_0
@username_2 @username_3 I did some new changes and tried to increase consistency and simplicity. Also I considered all of Jibé review points. So, do me a favor and check it out.
comment
created
50bhan
username_0
@username_2 @username_3 I think we are good to go. Final check? :laughing:
comment
created
50bhan
username_0
@username_3 Thank you :) Actually this one you mentioned is fixed already https://github.com/username_1/phpinsights/blob/25d73931b7afaf23248d441f9c818e8dc655a800/src/Application/Console/Definitions/AnalyseDefinition.php#L22
comment
created
Jibbarth
username_3
@username_0 no it's not :sweat_smile: I suggested to replace `The paths or files to analyse` by `Paths of directories or files to analyse` :wink:
comment
created
50bhan
username_0
@username_3 Yeah you are right :laughing: Done :+1:
comment
created
olivernybroe
username_2
@username_0 Have one bug we need to fix, but then I think this is ready to get merged in.

Running the following command, results in one file scanned
```bash
bin/phpinsights analyse src/Domain/Analyser.php
```

However running it like this, results in 4 files scanned and not 2.
```bash
bin/phpinsights analyse src/Domain/Analyser.php src/Application/Composer.php
```

It looks like it's a bug in `LocalFilesRepository::within`.
comment
created
50bhan
username_0
@username_2 That's weird! I'll check and let you know.
comment
created
50bhan
username_0
@username_2 I can't reproduce that! Here is my result:

![Screenshot_2020-04-02_17-49-30](https://user-images.githubusercontent.com/36772278/78254145-b4b36580-750a-11ea-8068-6d46d215c86f.png)
comment
created
50bhan
username_0
@username_2 It was a nice bug actually! You gave these two files:

`bin/phpinsights analyse src/Domain/Analyser.php src/Application/Composer.php`

but the result is:
`src/Application/Composer.php`
`src/Domain/Metrics/Architecture/Composer.php`

`src/Domain/Analyser.php`
`src/Application/Console/Analyser.php`

Because Symfony finder will search in `src/Domain/` and `src/Application/` directories and match with every files with names `Analyser.php` or `Composer.php`. So, I had to filter the final result with full path of files to fix it.
comment
created
olivernybroe
username_2
@username_0 Just tested it out again, and everything seems to work as expected now!

Nice debugging of the bug, that really was a nasty one of them 👍 

I'll merge this in now. 
Thanks a lot for the awesome contributions you keep making @username_0 ❤️ 

And thanks @username_3 for the amazing and always on point reviews!
comment
created
50bhan
username_0
@username_2 Anytime:) Really happy to contribute! After all I use insights in my personal and work projects, So it's awesome to be a small part of it. Thank you for your usual help and @username_3 for amazing reviews.
comment