Let phpcsfixer2 select the correct files and improve performance by veewee · Pull Request #386 · phpro/grumphp

veewee · 2017-07-28T14:37:15Z

Q	A
Branch	master for features and deprecations
Bug fix?	yes/no
New feature?	no
BC breaks?	yes
Deprecations?	no
Documented?	yes
Fixed tickets	#371, #327

veewee · 2017-07-28T14:38:50Z

Hi @Landerstraeten , @keradus,

This is an attempt to fix the issues in #371 and #327.
Can you give it a review to make sure this works as expected?

Thanks!

keradus · 2017-07-28T18:43:59Z

spec/Task/PhpCsFixerV2Spec.php

        $result->isPassed()->shouldBe(true);
    }

-    function it_runs_the_suite_for_changed_files(


why dropping the test instead of replacing it with test of new feature ?

keradus · 2017-07-28T18:56:57Z

doc/tasks/phpcsfixer2.md


+Intersection mode can only be used when you have a configuration file which contains a Finder.
+This mode works best since only files that are being commit and are in your configuration will be checked.
+When there is no Finder in your configuration, all committed files will be checked.


when there is no finder in configuration, setting path-mode=intersection will crash the execution

keradus · 2017-07-28T18:57:39Z

src/Task/PhpCsFixerV2.php

+        $arguments->addOptionalArgument('--path-mode=intersect', $config['can_intersect']);
        $arguments->addOptionalArgument('--verbose', $config['verbose']);
        $arguments->addOptionalArgument('--diff', $config['diff']);
        $arguments->add('fix');


did you try this out? please add tests to cover this change, as it is not working.

-intersect +intersection

We currently don't have a system to do e2e tests with 3rd party tools. I looked over that one during commit and will test these changes thoroughly before merging.

veewee · 2017-07-30T12:23:37Z

Thanks for your feedback @keradus!

I've changed the logic a bit:

Run context with can_intersect=true will never receive the files since the finder in the php-cs-fixer2 config can be used. (Works better on windows since they have a limited CLI input charakters)
Run context with can_intersect=false will always retrieve the files since there is no finder in configuration
PreCommit context with can_intersect=true will always retrieve the files and will run in intersection path mode. When there is no finder in the config file, the exectuable will fail.
PreCommit context with can_intersect=false will always retrieve the files and will run in override path mode

I assume this will cover all cases at the moment, but will test this thoroughly.

keradus · 2017-07-30T15:50:58Z

doc/tasks/phpcsfixer2.md

 This mode works best since only files that are being commit and are in your configuration will be checked.
-When there is no Finder in your configuration, all committed files will be checked.
+When there is no Finder in your configuration, you'll have set this parameter to false. 
+Otherwise php-cs-fixer will crash the execution.


btw, could there be consistency with naming? sometimes you use PHP-CS-Fixer, sometimes you use php-cs-fixer after binary file, while official name is PHP Coding Standards Fixer or PHP CS Fixer when one used shorter version.

Sure, I'll try to keep that in mind.

keradus · 2017-07-30T15:56:56Z

Run context with can_intersect=true will never receive the files since the finder in the php-cs-fixer2 config can be used. (Works better on windows since they have a limited CLI input charakters)

Run context with can_intersect=false will always retrieve the files since there is no finder in configuration

Then, what is the purpose of passing files in grumphp integration together with can_intersect=true ?

you are trying to use some extra logic on top of integration changing decision of user.
if user passed files, let use it.
if user configure mode=intersection, let use it.
that's it

veewee · 2017-07-31T18:46:18Z

Then, what is the purpose of passing files in grumphp integration together with can_intersect=true ?
you are trying to use some extra logic on top of integration changing decision of user.
if user passed files, let use it.
if user configure mode=intersection, let use it.
that's it

I disagree:

In run context, all files are being checked. That means that the task contains ALL files known to git. It is a bit overkill to paste them all the the CLI when there is a finder config in PHP CS Fixer. Besides that: Windows can't handle too many input characters.
So as long as there is a finder in configuration (can_intersect=true), we shouldn't pass the files in run context in my opninion.

The PreCommit context only contains the changed files. In this context, the intersection path-mode is the best option. If this is not possible (can_intersect=false), the override path-mode will be used.

Does this makes sense to you or would you still drop that logic?

keradus · 2017-07-31T18:52:12Z

So as long as there is a finder in configuration (can_intersect=true), we shouldn't pass the files in run context in my opninion.

so, why this scenario is allowed by code ?

veewee · 2017-07-31T18:56:26Z

I don't really understand your question. Can you give me some additional context?

keradus · 2017-07-31T19:30:07Z

    if ($context instanceof GitPreCommitContext || !$config['can_intersect']) {
        $arguments->addFiles($files);
    }

let discuss not-precommit context, thus we have:

    if (!$config['can_intersect']) {
        $arguments->addFiles($files);
    }

vs

In run context, all files are being checked.

No, looking at code, in run-context, the paths are sometimes passed, sometimes not

keradus · 2017-07-31T19:33:38Z

could can_intersect be different for run-context and precommit-context?

veewee · 2017-08-01T04:51:30Z

@keradus, I see what you are trying to say. Probably I explained incorrect:

can_intersect can't be different for run or pre-commit context, it is always the same since it is in configration. Maybe a better name would be config_contains_finder or something like that.

So lets asume can_intersect will be renamed to config_contains_finder. In that case you have this if:

    if ($context instanceof GitPreCommitContext || !$config['config_contains_finder']) {
        $arguments->addFiles($files);
    }

Let's focus on RunContext:

    if (!$config['config_contains_finder']) {
        $arguments->addFiles($files);
    }

As you can see: the files are being added when there is no finder configuration. If we don't do this, PHP CS Fixer will crash since it isn't validating any files.
When there is a finder in configuration, this one can be used instead of adding all files. This is because it should run on all files configured in the finder anyway.

Off course: for PreComit context, we'll always pass the files since we only want to check a limited subset.

keradus · 2017-08-01T11:51:37Z

cleaner to me, yet I do still not clear enough.

RunContext:
As you can see: the files are being added when there is no finder configuration. If we don't do this, PHP CS Fixer will crash since it isn't validating any files.

great, I do agree that if there is no finder, we shall pass the files.

then, you have execution scenario:

context: RunContext
config_contains_finder: true

what is gonna happen with current implementation?
you gonna configure --path-mode=intersection yet no files for it, and intersecting finder with empty set is... empty set. no files gonna be fixed

Landerstraeten · 2017-08-09T13:45:25Z

doc/tasks/phpcsfixer2.md



-**path_mode**
+**can_intersect**


I think you forgot to change the example on line 25

Landerstraeten · 2017-08-09T13:56:54Z

resources/config/tasks.yml

        arguments:
          - '@config'
          - '@process_builder'
-          - '@async_process_runner'


You deleted the argument from another task. Delete line 244 instead.

Landerstraeten · 2017-08-09T14:14:40Z

doc/tasks/phpcsfixer2.md

+Intersection mode can only be used when you have a configuration file which contains a Finder.
+This mode works best since only files that are being commit and are in your configuration will be checked.
+When there is no Finder in your configuration, you'll have set this parameter to false. 
+Otherwise php-cs-fixer will crash the execution.


This is clear 👍

Landerstraeten

By default, I wasn't able to run it correctly. I had some issues during a sanity check. I have left some remaks to fix it.

veewee · 2017-09-01T06:07:39Z

Fixed in #395

Let phpcsfixer2 select the correct files and improve performance

7f9ae3b

veewee added the enhancement label Jul 28, 2017

veewee added this to the Version 0.12.0 milestone Jul 28, 2017

veewee requested a review from Landerstraeten July 28, 2017 14:37

keradus reviewed Jul 28, 2017

View reviewed changes

Improve php-cs-fixer2 task execution

217c6e3

keradus reviewed Jul 30, 2017

View reviewed changes

Landerstraeten reviewed Aug 9, 2017

View reviewed changes

doc/tasks/phpcsfixer2.md

**path_mode**

**can_intersect**

Copy link

Contributor

Landerstraeten Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to change the example on line 25

Landerstraeten reviewed Aug 9, 2017

View reviewed changes

Landerstraeten suggested changes Aug 9, 2017

View reviewed changes

Landerstraeten mentioned this pull request Aug 25, 2017

Fix phpcsfixer2 issues #395

Merged

veewee closed this Sep 1, 2017

Conversation

veewee commented Jul 28, 2017

Uh oh!

veewee commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veewee commented Jul 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keradus commented Jul 30, 2017

Uh oh!

veewee commented Jul 31, 2017

Uh oh!

keradus commented Jul 31, 2017

Uh oh!

veewee commented Jul 31, 2017

Uh oh!

keradus commented Jul 31, 2017

Uh oh!

keradus commented Jul 31, 2017

Uh oh!

veewee commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keradus commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Landerstraeten left a comment

Choose a reason for hiding this comment

Uh oh!

veewee commented Sep 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

veewee commented Aug 1, 2017 •

edited

Loading

keradus commented Aug 1, 2017 •

edited

Loading