Enable include_paths #16
Conversation
| $dir = rtrim($dir, '\\/'); | ||
|
|
||
| foreach (scandir($dir) as $f) { | ||
| if (in_array("$prefix$f", $this->config["exclude_paths"])) { | ||
| if (!isset($this->config['include_paths']) && in_array("$prefix$f", $this->config["exclude_paths"])) { |
There was a problem hiding this comment.
I get why this works but it took me a minute. I appreciate the desire to reuse the file expansion logic, but is there a way to do it without having to resort to checks like this?
There was a problem hiding this comment.
Hmm while it's not the most optimal solution, it allows for a nice code reuse, just like you noticed.
I am not super comfortable with php so it is unlikely I will come up with anything better. Although, if you guys have a better design in mind, I could try to reimplement it with your guidance.
There was a problem hiding this comment.
How about passing in the exclusions as an array when you call queuePaths? That way the exclude_paths-based option could pass in $this->config["exclude_paths"], while the include-paths-based option could pass in an empty array.
There was a problem hiding this comment.
Sorry for the delayed reply. To be honest I am not sure if I got your idea right. How would the include_paths be handled if I pass an empty array? It's worth mentioning that at the moment, an engine decides to expand include_paths when this config value is not empty and the way CLI works after the recent changes is that it translates exclude_paths to include_paths.
As a result include_paths are the ones always getting expanded.
There was a problem hiding this comment.
Really my design criticism is that it seems that queuePaths has too much responsibility. By this time we enter queuePaths, we've already decided whether we're using includes or excludes, so checking for them here again just makes the code harder to read.
So, my suggestion was, if you want to make queuePaths reusable, you could take exclusions in the argument list:
public function queuePaths($dir, $prefix = '', $exclusions = []) {
And remove the explicit references to $this->config in line 50:
if (in_array("$prefix$f", $exclusions)) {
Which means that you could use this for exclude-based file building this way:
$this->queuePaths($dir, $prefix, $this->config['exclude_paths'])
But for include-based file building you could leave line 32 as:
$this->queuePaths("/code$f", "$f/");
In that case, $exclusions will simply be an empty array, and will not skip any files in the directory. But you get the goal of reusing queuePaths while focusing its responsibilities.
|
Thanks @fhwang for your suggestions, I've modified the code. |
|
Other then the CC issues, LGTM |
|
What are the CC issues? I can't access that page. |
|
@jacobi007 Hm when I click on the link to CC issues, I don't see any new ones:
So I think we can disregard those for now. Anything else blocking merging this? |
|
cc @codeclimate/review |
| foreach ($this->config['include_paths'] as $f) { | ||
| if ($f !== '.' and $f !== '..') { | ||
|
|
||
| if (is_dir("/code$f")) { |
There was a problem hiding this comment.
I didn't think we wrote include_paths with leading / chars. I notice you do put a / before $f on line 39. I'm guessing this has been tested, but just checking it's intentional.
|
Yeah, the code climate issues thing is a bug on our end. I've seen it before. 😢 |
|
Looks good based from some testing. |
As listed in spec