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 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)

This comment has been minimized.

Copy link
@keradus

keradus Sep 25, 2014

Member

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

This comment has been minimized.

Copy link
@fivestar

fivestar Sep 25, 2014

Author Contributor

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

This comment has been minimized.

Copy link
@keradus

keradus Sep 25, 2014

Member

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

* with this source code in the file LICENSE.
*/

namespace Symfony\CS\Console;

This comment has been minimized.

Copy link
@keradus

keradus Sep 25, 2014

Member

You use it in console, but it does not do anything with console. It should not be in Console namespace

This comment has been minimized.

Copy link
@fivestar

fivestar Sep 25, 2014

Author Contributor

Certainly this class is not dependent on Console, but this class handles CLI options (especially $filterOption).
Considering the task for this class, so I think this class have to be under the Console namespace.

Where is better? Util\ or else? Tell me what you think!

This comment has been minimized.

Copy link
@keradus

keradus Sep 25, 2014

Member

I would agree if it manipulate only on console inputs. But it handle also Config and Fixer (or only Config, if you changed it) classes.
I would put it into Symfony\CS directry. But that's only my opinion.


public function resolve($levelOption, $fixersOption)
{
$allFixers = $this->fixer->getFixers();

This comment has been minimized.

Copy link
@keradus

keradus Sep 25, 2014

Member

IMO this should be passed into class - the defined Fixers, not Fixer itself.

This comment has been minimized.

Copy link
@fivestar

fivestar Sep 25, 2014

Author Contributor

Okay.

@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) {

This comment has been minimized.

Copy link
@keradus

keradus Sep 25, 2014

Member

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

This comment has been minimized.

Copy link
@fivestar

fivestar Sep 25, 2014

Author Contributor

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 = [];

This comment has been minimized.

Copy link
@keradus

keradus Sep 25, 2014

Member

crash on php5.2

This comment has been minimized.

Copy link
@stof

stof Oct 4, 2014

Contributor

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

This comment has been minimized.

Copy link
@keradus

keradus Oct 4, 2014

Member

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!

@keradus keradus added the kind/bug label Sep 30, 2014
@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 fivestar:resolve-mixed-fixtures branch 4 times, most recently from 343be2f to 45a1c1f 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);

This comment has been minimized.

Copy link
@keradus

keradus Oct 6, 2014

Member

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)

This comment has been minimized.

Copy link
@keradus

keradus Oct 27, 2014

Member

already done.

if (null === $levelOption
&& (empty($fixersOption) || preg_match('{(^|,)-}', $fixersOption))
) {
$levelOption = $config->getFixers();

This comment has been minimized.

Copy link
@keradus

keradus Oct 6, 2014

Member

I would break a BC here - let the Config::getFixers returns fixers array and Config::getLevel returns level constant.
It will not be confusing then.

This comment has been minimized.

Copy link
@keradus

keradus Oct 27, 2014

Member

already done.


if (is_string($levelOption)) {
switch ($levelOption) {
case 'psr0':

This comment has been minimized.

Copy link
@keradus

keradus Oct 6, 2014

Member

just do the mapping:
static array $foo = array('psr0' => FixerInterface::PSR0_LEVEL, ...);

This comment has been minimized.

Copy link
@keradus

keradus Oct 27, 2014

Member

already done.

}

// select base fixers for the given level
if (is_array($level)) {

This comment has been minimized.

Copy link
@keradus

keradus Oct 6, 2014

Member

$level should not be an array of fixers, array of fixers should be in $fixers.
This will lead to breaking BC as mentioned before.

This comment has been minimized.

Copy link
@keradus

keradus Oct 27, 2014

Member

already done.

$removeNames = array();
foreach ($names as $name) {
if (0 === strpos($name, '-')) {
$removeNames[] = ltrim($name, '-');

This comment has been minimized.

Copy link
@keradus

keradus Oct 6, 2014

Member

I would use ~map here instead of array (in $addNames the same):
$removeNames[$name] = true

then you will not need to chek in_array($name $removeNames, true) but just isset($removeNames[$name])

This comment has been minimized.

Copy link
@keradus

keradus Oct 6, 2014

Member

btw, with larges arrays it would be a little faster i think

This comment has been minimized.

Copy link
@keradus

keradus Oct 27, 2014

Member

already done.

return $fixers;
}

public function resolveByNames(array $fixers, $names)

This comment has been minimized.

Copy link
@keradus

keradus Oct 6, 2014

Member

I think array $fixers is a state of this class, it should not be passed in and out.
calculate it inside class and make a getter for it.

This comment has been minimized.

Copy link
@keradus

keradus Oct 27, 2014

Member

already done.


foreach ($fixers as $fixer) {
switch ($fixer->getName()) {
case 'encoding': // psr1

This comment has been minimized.

Copy link
@keradus

keradus Oct 6, 2014

Member

And what if sb change this name of Fixer?
Please get it from fixer class itself.

This comment has been minimized.

Copy link
@keradus

keradus Oct 6, 2014

Member

oh, you have it in line 44 already.
ok, no big deal

@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 fivestar:resolve-mixed-fixtures branch from bc4c879 to 97a8abf 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.

This comment has been minimized.

Copy link
@sstok

sstok Oct 29, 2014

Contributor

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

@@ -197,6 +202,23 @@ protected function configure()
?>
The ``symfony`` level is set by default, you can also change default level:

This comment has been minimized.

Copy link
@sstok

sstok Oct 29, 2014

Contributor

you can also change the default level

@sstok
Copy link
Contributor

sstok commented Oct 29, 2014

👍 for this feature.

@fivestar fivestar force-pushed the fivestar:resolve-mixed-fixtures branch from 97a8abf to 23c07bb 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 FriendsOfPHP:master Nov 2, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
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 fivestar: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
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.