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

Fix fix command to handle --fixers option when contains extra and excluding fixers #599

Merged
merged 4 commits into from Nov 2, 2014

Conversation

fivestar
Copy link
Contributor

@fivestar fivestar commented Sep 25, 2014

Currently, when --fixers option contains extra and excluding mixed filters such as -eol_ending,concat_with_spaces, excluding filter will be enabled, however, extra filter will not be enabled.

This PR fixes this behavior, so enable both type filters when mixed filters given.


sample.php

<?php
echo 'foo'.'bar', PHP_EOL;

Current version: (concat_with_spaces is not enabled)

% php-cs-fixer fix -v --diff --fixers="-eol_ending,concat_with_spaces" --dry-run sample.php   
   1) /home/fivestar/sample.php (eof_ending)
      ---------- begin diff ----------
      --- Original
      +++ New
      @@ @@
       <?php
       echo 'foo'.'bar', PHP_EOL;

      -

      ---------- end diff ----------

Fixed version: (concat_with_spaces is enabled)

% php-cs-fixer fix -v --diff --fixers="-eol_ending,concat_with_spaces" --dry-run sample.php 
   1) /home/fivestar/sample.php (eof_ending, concat_with_spaces)
      ---------- begin diff ----------
      --- Original
      +++ New
      @@ @@
       <?php
      -echo 'foo'.'bar', PHP_EOL;
      -
      +echo 'foo' . 'bar', PHP_EOL;


      ---------- end diff ----------

$this->config = $config;
}

public function resolve($levelOption, $fixersOption)
Copy link
Member

@keradus keradus Sep 25, 2014

Choose a reason for hiding this comment

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

Really, create extra class and do everything in one, the only one method?

Copy link
Contributor Author

@fivestar fivestar Sep 25, 2014

Choose a reason for hiding this comment

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

Why create extra class is because to get testability and separate responsibilities.

Copy link
Member

@keradus keradus Sep 25, 2014

Choose a reason for hiding this comment

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

Cool! but why only one magic method to do all things?

@keradus
Copy link
Member

keradus commented Sep 25, 2014

Maybe this is good plase to separate Config::$fixers into two values?
One that handle LEVEL, and second that handle fixers array? Now this props handle both :(

$addNames = [];
$removeNames = [];
foreach ($names as $name) {
if (strpos($name, '-') === 0) {
Copy link
Member

@keradus keradus Sep 25, 2014

Choose a reason for hiding this comment

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

please use Symfony standards here: 0 === $sth

Copy link
Contributor Author

@fivestar fivestar Sep 25, 2014

Choose a reason for hiding this comment

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

I see.

@keradus
Copy link
Member

keradus commented Sep 25, 2014

Idea itself looks nice!
It would be great to add info about levels/fixers and mixing them (with excluding/including extra fixers) into readme.
I know that this is not intuitive now.

@keradus
Copy link
Member

keradus commented Sep 25, 2014

Fix fix command to handle --filter option

--filter? where?


// remove/add fixers based on the fixers option
$names = array_map('trim', explode(',', $fixersOption));
$addNames = [];
Copy link
Member

@keradus keradus Sep 25, 2014

Choose a reason for hiding this comment

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

crash on php5.2

Copy link
Contributor

@stof stof Oct 4, 2014

Choose a reason for hiding this comment

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

on PHP 5.3, not on 5.2 (the whole library crashes on 5.2 because it uses namespaces)

Copy link
Member

@keradus keradus Oct 4, 2014

Choose a reason for hiding this comment

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

yes, @fivestar point it out 9 days ago and I already sorry for my typo ;)

@fivestar fivestar changed the title Fix fix command to handle --filter option when contains extra and excluding filters Fix fix command to handle --fixers option when contains extra and excluding filters Sep 25, 2014
@fivestar
Copy link
Contributor Author

fivestar commented Sep 25, 2014

Thanks for your comments! I replied some line notes.

  • Sorry --filter is mistaken. --fixers is right. Do i have to amend commit message?
  • Will be fix php5.2 (5.3?) crash.
  • Maybe this is good plase to separate Config::$fixers into two values?
    One that handle LEVEL, and second that handle fixers array? Now this props handle both :(

    • Do you have any ideas? Maybe current code has any problems?
  • It would be great to add info about levels/fixers and mixing them (with excluding/including extra fixers) into readme.

    • I'll see what I can do.

@keradus
Copy link
Member

keradus commented Sep 25, 2014

Do i have to amend commit message?

If you will keep this PR with only one commit then please, do it. If PR will has several then I will do it during merge.

Will be fix php5.2 (5.3?) crash.

Yeah, 5.3, sorry ;)

Maybe this is good plase to separate Config::$fixers into two values?

Do you have any ideas? Maybe current code has any problems?

No problems on that, just now we have one variable with dual responsibility. You are dealing with them now so it will be cool to fix it as well ;) Just create Config::$level for level value and change Config::$fixers to handle only fixers array

@keradus keradus changed the title Fix fix command to handle --fixers option when contains extra and excluding filters Fix fix command to handle --fixers option when contains extra and excluding fixers Sep 25, 2014
@keradus
Copy link
Member

keradus commented Sep 26, 2014

Please rebase on master ;)

@keradus
Copy link
Member

keradus commented Sep 30, 2014

any news @fivestar ? The work you have already done is cool!

@fivestar
Copy link
Contributor Author

fivestar commented Oct 1, 2014

Sorry, I've been so busy lately... Please wait for a little while longer!

@fivestar fivestar force-pushed the resolve-mixed-fixtures branch 4 times, most recently from 343be2f to 45a1c1f Compare Oct 6, 2014
@fivestar
Copy link
Contributor Author

fivestar commented Oct 6, 2014

@keradus Please check it!

}
}
$fixers = $fixersResolver->resolveByLevel($levelOption);
$fixers = $fixersResolver->resolveByNames($fixers, $fixersOption);
Copy link
Member

@keradus keradus Oct 6, 2014

Choose a reason for hiding this comment

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

So we have here sth like:

$fixersResolver = new FixersResolver($this->fixer->getFixers());
$fixers = $fixersResolver->resolveByLevel($levelOption);
$fixers = $fixersResolver->resolveByNames($fixers, $fixersOption);

So we passing $fixersOption, $levelOption, $fixers and $fixersOption, where$levelOptionis calculated here from$config->getFixers();or input itself. We also calculate sth fromFixersResolverand passing it toFixerResolver` again - just use a state.

It would be great if you have some setters to set $level and $fixers from input/config, then resolveBy* would be a private methods and at finish FixersResolver will have some public method like getFixers/resolve (which do all the magic like parsing inputs and calling private helpers)

Copy link
Member

@keradus keradus Oct 27, 2014

Choose a reason for hiding this comment

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

already done.

@keradus
Copy link
Member

keradus commented Oct 6, 2014

Nice progress @fivestar ;)
But I think it needs just a little more (see code comments)

@keradus
Copy link
Member

keradus commented Oct 9, 2014

It would be great to see that on master! Waiting for progress ;)

@keradus
Copy link
Member

keradus commented Oct 28, 2014

I test this branch a lot, results are below.


             CONFIG      OPTION
LEVEL           X
FIXERS

Result: Take level from config.
OK


             CONFIG      OPTION
LEVEL           X
FIXERS          X

Result:
If fixers from config have only "positive" fixers: Take fixers from config and ignore level from config.
If fixers from config have "negative" fixers: Take level from config and apply fixers from config as patch.
ERROR: In both cases the fixers from config should be applied as patch.


             CONFIG      OPTION
LEVEL           X           X
FIXERS

Result: Take level from option.
OK


             CONFIG      OPTION
LEVEL           X           X
FIXERS          X           X

Result: Take level from option and apply fixers from options as patch.
OK


             CONFIG      OPTION
LEVEL           X
FIXERS          X           X

Result:
If fixers from options have only "positive" fixers: Take fixers from option and ignore level from config.
If fixers from options have "negative" fixers: Take level from config and apply fixers from option as patch.
OK


             CONFIG      OPTION
LEVEL           X           X
FIXERS          X

Result: Take level from option and ignore level from config. Then apply fixers from config as patch.
ERROR: Fixer from config should not be applied.


When sb run php-cs-fixer fix --level=symfony then one want to run only the level he set, without fixers from config as patch

@keradus
Copy link
Member

keradus commented Oct 28, 2014

@fabpot @fivestar @stof @sstok @GrahamCampbell
what do you think about above strategy of dealing with level/fixers config/options ?

BTW, tests and solution for that behavior are availeble here: fivestar#2

@fivestar
Copy link
Contributor Author

fivestar commented Oct 28, 2014

Thanks!! I misread your specified commit number so I'll squash after merging your PR ;)

@fivestar
Copy link
Contributor Author

fivestar commented Oct 29, 2014

I've seen test results and your PR, so I think your strategy is better because it's is useful for actual use case.
If you judge your PR can merge, please call me again.

@keradus
Copy link
Member

keradus commented Oct 29, 2014

I am not PR-ing with code I don't want to be merged ;)
(Sth I use WIP flag to show that PR is not done yet).

If you agreed with changes just accept that PR (fivestar#2). I don't change the whole strategy - just fix 2 cases.

The other thing is if this PR (#599) will be merged - and I will be glad to see more opinions here.
For me it should be ;)

@fivestar fivestar force-pushed the resolve-mixed-fixtures branch from bc4c879 to 97a8abf Compare Oct 29, 2014
@fivestar
Copy link
Contributor Author

fivestar commented Oct 29, 2014

Ok I merged your PR. (What is WIP? I can't find the keyword)
What should I do sth else?

@keradus
Copy link
Member

keradus commented Oct 29, 2014

  • WIP stands for Work In Progress
  • 6f0cb71 and 97a8abf should be squashed
  • I want to wait a little for some other opinions about this PR. I will be glad to see them.

@fivestar
Copy link
Contributor Author

fivestar commented Oct 29, 2014

Uh oh I thought you are judged. I'll remove commits later. Please resend PR.

@keradus
Copy link
Member

keradus commented Oct 29, 2014

I'll remove commits later. Please resend PR.

Why to remove? The new commits are good - they improve this PR (#599).
Just please squash them.

Uh oh I thought you are judged.

Yes, I can perform a merge. But since this PR change tool interface and break BC it is worthly to give community a time to give a feedback with opinion :)

@ghost
Copy link

ghost commented Oct 29, 2014

👍


In combination with these config and command line options, you can choose various usage.

For example, default level is ``symfony``, but if you also don't want to use ``psr0`` fixer, you can specify ``--fixers="-psr0"`` option.
Copy link
Contributor

@sstok sstok Oct 29, 2014

Choose a reason for hiding this comment

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

but if you also don't want to use the psr0 fixer.

you can specify the --fixers="-psr0" option.

And try to wrap lines a bit ;) approx 80 chars max

@sstok
Copy link
Contributor

sstok commented Oct 29, 2014

👍 for this feature.

@fivestar fivestar force-pushed the resolve-mixed-fixtures branch from 97a8abf to 23c07bb Compare Oct 29, 2014
@fivestar
Copy link
Contributor Author

fivestar commented Oct 29, 2014

I've just got it! I squashed some commits.

@sstok Thanks for your help! I fixed some messages.

@alterphp
Copy link

alterphp commented Oct 31, 2014

👍 great job !

@keradus
Copy link
Member

keradus commented Nov 2, 2014

Thank you @fivestar.

@keradus keradus merged commit 23c07bb into PHP-CS-Fixer:master Nov 2, 2014
keradus added a commit that referenced this pull request Nov 2, 2014
…ins extra and excluding fixers (fivestar, keradus)

This PR was merged into the 1.0.x-dev branch.

Discussion
----------

Fix `fix` command to handle `--fixers` option when contains extra and excluding fixers

Currently, when `--fixers` option contains extra and excluding mixed filters such as `-eol_ending,concat_with_spaces`, excluding filter will be enabled, however, extra filter will not be enabled.

This PR fixes this behavior, so enable both type filters when mixed filters given.

----

`sample.php`

```php
<?php
echo 'foo'.'bar', PHP_EOL;

```

Current version: (`concat_with_spaces` is not enabled)

```
% php-cs-fixer fix -v --diff --fixers="-eol_ending,concat_with_spaces" --dry-run sample.php
   1) /home/fivestar/sample.php (eof_ending)
      ---------- begin diff ----------
      --- Original
      +++ New
      @@ @@
       <?php
       echo 'foo'.'bar', PHP_EOL;

      -

      ---------- end diff ----------
```

Fixed version: (`concat_with_spaces` is enabled)

```
% php-cs-fixer fix -v --diff --fixers="-eol_ending,concat_with_spaces" --dry-run sample.php
   1) /home/fivestar/sample.php (eof_ending, concat_with_spaces)
      ---------- begin diff ----------
      --- Original
      +++ New
      @@ @@
       <?php
      -echo 'foo'.'bar', PHP_EOL;
      -
      +echo 'foo' . 'bar', PHP_EOL;

      ---------- end diff ----------
```

Commits
-------

23c07bb FixersResolver - make class more state aware and enhance it's tests
e75c985 Fix README and some codes.
6876726 FixersResolver - simplify code
f3c966f Fix `fix` command to handle `--fixers` option when contains include and excluding fixers
@keradus keradus mentioned this pull request Nov 3, 2014
@fivestar
Copy link
Contributor Author

fivestar commented Nov 4, 2014

Thank you too!!

@fivestar fivestar deleted the resolve-mixed-fixtures branch Nov 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants