Make php extensions configurable #83

paolomainardi · 2017-07-11T16:14:00Z

No description provided.

zdenekdrahos · 2017-07-12T09:42:22Z

Good idea to configure extensions 👍

I'd rather move the configuration to .phpqa.yml. I'd like to keep CLI options as simple as possible. Option --config is used for defining an advanced configuration. Is that ok for you?

zdenekdrahos · 2017-07-12T09:44:44Z

src/Options.php

@@ -46,6 +49,7 @@ function ($dir) {
        $this->isOutputPrinted = $this->isSavedToFiles ? $options['verbose'] : true;
        $this->hasReport = $this->isSavedToFiles ? $options['report'] : false;
        $this->configDir = $options['config'] ? $options['config'] : getcwd();
+        $this->extensions = isset($options['extensions']) ? $options['extensions'] : 'php';


isset is not necessary when you update copy-pasted code in OptionsTest
It's complicated to retrieve default options from comment, so they are copy-pasted in the test

You don't have to fix it. As I said in the comment, I would prefer configuration in .phpqa.yml.
You can take a look at adding configuration to .phpqa.yml.

paolomainardi · 2017-07-12T15:52:04Z

@zdenekdrahos done, you can review it now.

zdenekdrahos

Make extensions configurable globally. Then only three lines should be changed in CodeAnalysisTasks

zdenekdrahos · 2017-07-13T07:15:15Z

README.md

@@ -139,6 +139,7 @@ For full documentation please visit [eko3alpha/docker-phpqa](https://hub.docker.
 | ~~`phpqa --analyzedDir ./`~~ | Deprecated in **v1.8** in favor of `--analyzedDirs` |
 | `phpqa --ignoredDirs build,vendor` | Ignore directories |
 | `phpqa --ignoredFiles RoboFile.php` | Ignore files |
+| `phpqa --extensions php,inc,module` | Run only on selected extensions (default: ``php`` |


Old comment, extensions should be documented in advanced configuration

zdenekdrahos · 2017-07-13T07:17:43Z

.phpqa.yml

@@ -11,6 +11,8 @@ phpcs:
            checkstyle: checkstyle.xml
            # you can include custom reports (https://github.com/wikidi/codesniffer/blob/master/reports/wikidi/Summary.php#L39)
            # ./vendor/owner/package/src/MySummaryReport.php: phpcs-summary.html
+    extensions:


I wouldn't define extensions for each tool, but only once (just like you had it in --extensions).

extensions: - php

zdenekdrahos · 2017-07-13T07:25:43Z

src/CodeAnalysisTasks.php

            'standard' => $standard,
            $this->options->ignore->phpcs(),
            $this->options->getAnalyzedDirs(' '),
        );
+        if (!empty($extensions)) {


There is a lot of duplication in loading extensions. Extracting the code to Config might be a good idea.

$args = array( 'extensions' => $this->config->csv('extensions'), );

It would be great to have tests that show behavior what happens, when extensions is array, string, ...
I would ignore cases when extensions are empty. It's required configuration, so we don't have to be too defensive.

paolomainardi · 2017-07-13T11:11:24Z

@zdenekdrahos thanks for the review, you can see latest changes now.

fix notice refactoring using .phpqa.yml move extensions to global namespace, add test, add logic to Config class to parse the array fix metrics

zdenekdrahos

A couple small fixes and it's ready to merge.
Good job 👍

zdenekdrahos · 2017-07-13T12:05:03Z

src/CodeAnalysisTasks.php

@@ -281,15 +281,17 @@ private function buildPhpcs(RunningTool $tool, array $installedStandards)

    private function pdepend()
    {
-        return array(
+        $opts = array(


Please revert a return. The temporary variable is useless.

zdenekdrahos · 2017-07-13T12:11:27Z

tests/Config/ConfigTest.php

@@ -56,6 +58,26 @@ public function testThrowExceptionWhenFileDoesNotExist()
        $config->path('phpcs.standard');
    }

+    public function testConfigCsvArray()


One test is good enough if you move php,inc,module to tests/Config/.phpqa.yml.

Array format is tested in default config.

Hardcoding expected value in the test is fine (not perfect, but the test is more readable without parsing YAML).

I guess that removing YAML::parse should help to pass Travis.ci

zdenekdrahos · 2017-07-13T12:14:09Z

README.md

@@ -21,7 +21,7 @@ If you ever tried to get ignoring directories to work then you know what I mean.
 CLI tools are cool because you can analyze any directory or file.


I see only whitespace changes. Revert changes if you don't plan to document extensions in advanced configuration.

Is has been automatically added by sublime, it is just a whitespace, i guess it quite safe to keep it.

I agree that it is a safe change. But should it be a part of "Make PHP extensions configurable" commit, if nothing else is changed?

You can keep it, but add documentation for extensions (newline after README.md#L201). Thanks

[extensions](https://github.com/EdgedesignCZ/phpqa/blob/master/.phpqa.yml) | File extensions | `[php]` | List of PHP file extensions |

paolomainardi · 2017-07-13T12:26:55Z

@zdenekdrahos done

zdenekdrahos · 2017-07-13T16:28:35Z

tests/Config/ConfigTest.php

@@ -56,6 +56,15 @@ public function testThrowExceptionWhenFileDoesNotExist()
        $config->path('phpcs.standard');
    }

+    public function testConfigCsvString()


Please load tests/Config/.phpqa.yml and remove tests/Config/data/custom-extensions-string/.phpqa.yml.

$config->loadCustomConfig(__DIR__);

isNonEmptyString assert is not necessary, equalTo catches all types of invalid values. When the test fails I would rather see, that expected value is php,inc,module instead of non-empty string. But that's a minor issue, you can leave it that way if you think it's better.

zdenekdrahos · 2017-07-13T16:41:29Z

README.md

@@ -21,7 +21,7 @@ If you ever tried to get ignoring directories to work then you know what I mean.
 CLI tools are cool because you can analyze any directory or file.


I agree that it is a safe change. But should it be a part of "Make PHP extensions configurable" commit, if nothing else is changed?

You can keep it, but add documentation for extensions (newline after README.md#L201). Thanks

[extensions](https://github.com/EdgedesignCZ/phpqa/blob/master/.phpqa.yml) | File extensions | `[php]` | List of PHP file extensions |

paolomainardi · 2017-07-13T17:04:42Z

@zdenekdrahos done 👍

zdenekdrahos · 2017-07-13T18:11:19Z

I don't like squash. But I did it because of general commit message "code review" in history. I hope that's okay.

#83

paolomainardi changed the title ~~Add extensions support~~ Make php extensions configurable Jul 11, 2017

zdenekdrahos reviewed Jul 12, 2017

View reviewed changes

zdenekdrahos requested changes Jul 13, 2017

View reviewed changes

add extensions support

33a7974

fix notice refactoring using .phpqa.yml move extensions to global namespace, add test, add logic to Config class to parse the array fix metrics

paolomainardi force-pushed the feature/make-php-extensions-configurable branch from 3afb72f to 33a7974 Compare July 13, 2017 12:06

zdenekdrahos requested changes Jul 13, 2017

View reviewed changes

code review

a05bb36

zdenekdrahos requested changes Jul 13, 2017

View reviewed changes

upgrade readme, fix test, remove test data

c53dd7d

zdenekdrahos approved these changes Jul 13, 2017

View reviewed changes

zdenekdrahos merged commit 506d340 into EdgedesignCZ:master Jul 13, 2017

zdenekdrahos added a commit that referenced this pull request Jul 14, 2019

phpqa - move configuring extensions to phpqa (no BC, only warning)

de736d6

#83

zdenekdrahos mentioned this pull request Jul 14, 2019

Advanced .phpqa.yml config - multiple phpcs standards, phpqa config #177

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make php extensions configurable #83

Make php extensions configurable #83

paolomainardi commented Jul 11, 2017

zdenekdrahos commented Jul 12, 2017

zdenekdrahos Jul 12, 2017

zdenekdrahos Jul 12, 2017

paolomainardi commented Jul 12, 2017

zdenekdrahos left a comment

zdenekdrahos Jul 13, 2017

zdenekdrahos Jul 13, 2017

zdenekdrahos Jul 13, 2017

paolomainardi commented Jul 13, 2017

zdenekdrahos left a comment

zdenekdrahos Jul 13, 2017

zdenekdrahos Jul 13, 2017

zdenekdrahos Jul 13, 2017

paolomainardi Jul 13, 2017

zdenekdrahos Jul 13, 2017

paolomainardi commented Jul 13, 2017

zdenekdrahos Jul 13, 2017

paolomainardi Jul 13, 2017

zdenekdrahos Jul 13, 2017

paolomainardi commented Jul 13, 2017

zdenekdrahos commented Jul 13, 2017

		@@ -21,7 +21,7 @@ If you ever tried to get ignoring directories to work then you know what I mean.
		CLI tools are cool because you can analyze any directory or file.

Make php extensions configurable #83

Make php extensions configurable #83

Conversation

paolomainardi commented Jul 11, 2017

zdenekdrahos commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paolomainardi commented Jul 12, 2017

zdenekdrahos left a comment

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

paolomainardi commented Jul 13, 2017

zdenekdrahos left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paolomainardi commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paolomainardi commented Jul 13, 2017

zdenekdrahos commented Jul 13, 2017