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

php-cs-fixer #11

Merged
merged 21 commits into from Apr 17, 2015

Conversation

@Hywan
Copy link
Member

Hywan commented Mar 6, 2015

This PR introduces:

  • hoa devtools:cs fix . to automatically run php-cs-fixer and automatically link to the following configuration file,
  • A configuration file,
  • New fixers:
    • phpdoc_var (fix @var),
    • phpdoc_access (remove @access),
    • php_throws (replace @throws by @throw),
    • author (remove `@author),
    • copyright (update and fix copyrights),
    • block (fix newlines in blocks),
    • return (fix newlines with return),
    • indent_concatenation (everything must be indented, even concatenation).

I let you take a look at the API documentation explaining the fixers.

Fix hoaproject/Websocket#25.
Fix hoaproject/Central#13.

@Hywan Hywan added the enhancement label Mar 6, 2015
@Hywan Hywan self-assigned this Mar 6, 2015
@Hywan Hywan force-pushed the Hywan:php-cs-fixer branch from 774429e to e7f53ad Mar 6, 2015
@Hywan

This comment has been minimized.

Copy link
Member Author

Hywan commented Mar 6, 2015

We must define how block works. Here is my proposal:

{
    a single statement;
}

{

    more;
    than;
    one;
    statement;

}

No newline after and before { and } if we have a single statement. Newlines if we have more than one statement. Concrete example:

if (true === $foo) {
    doSomething();
} else {

    $a++;
    $b = str_replace(…, …, …);

}

Now, how to treat the special case of the return statement? Does it count as a statement? The rule is: If we have 0 or 1 statement and a return, we do not add any newline. If we have more than 1 statement and a return, we add newline and an empty line before the return. Like this:

{

    a single statement;
    return;

}

{

    a statement;
    another statement;

    return;

}

Thoughts?

Update: break, continue and exit have the same rule as return.

@Hywan

This comment has been minimized.

Copy link
Member Author

Hywan commented Mar 6, 2015

@Hywan

This comment has been minimized.

Copy link
Member Author

Hywan commented Mar 6, 2015

Example of a PR: hoaproject/Websocket#34.
Need to retouch manually for the block and return family. Everything else is pretty much perfect (sometimes we have to fix some stuff manually, nothing horrible).

@Metalaka

This comment has been minimized.

Copy link
Member

Metalaka commented Mar 7, 2015

Looks good but I think you inverted the @throws tag name normalization, according to phpdoc.org and php-cs-fixer:phpdoc_params [symfony] @throw doesn't exist.

@Hywan

This comment has been minimized.

Copy link
Member Author

Hywan commented Mar 7, 2015

@Metalaka ;-) 👍

@Hywan

This comment has been minimized.

Copy link
Member Author

Hywan commented Apr 8, 2015

What about newlines in blocks? I am still not sure what to do…

@Hywan

This comment has been minimized.

Copy link
Member Author

Hywan commented Apr 15, 2015

I propose to not add newlines with the block contains one or more statements.

@Hywan

This comment has been minimized.

Copy link
Member Author

Hywan commented Apr 15, 2015

Done. PR is ready to be applied. I would like all @hoaproject/hoackers to review this result: https://github.com/hoaproject/Websocket/pull/34/files please :-). Report everything on this link, no this page.

Thanks!

Bin/Cs.php Outdated
* @access public
* @return int
*/
public function main ( ) {

This comment has been minimized.

Copy link
@Shine-neko
Bin/Cs.php Outdated
* @license New BSD License
*/
class Cs extends Console\Dispatcher\Kit {

This comment has been minimized.

Copy link
@Shine-neko

This comment has been minimized.

Copy link
@Hywan

Hywan Apr 16, 2015

Author Member

We didn't not apply the CS fixer on these files yet.

if (!$tokens[1]->isWhitespace()) {
$tokens[0]->setContent('<?php');
$tokens->insertAt(1, new Token([T_WHITESPACE, "\n\n"]));

This comment has been minimized.

Copy link
@stof

stof Apr 16, 2015

there is already a fixer doing that in the PHP-CS-Fixer: blankline_after_open_tag. I suggest using it instead of reimplementing your own, especially if you don't cover your implementation with tests.

And your current implementation is flawed here IIRC: it corrupts the token collection, which will impact other fixers because of the tokenization cache.
I suggest you to write tests by using the AbstractFixerTestBase of PHP-CS-Fixer, which now prevents such corruptions.

This comment has been minimized.

Copy link
@Hywan

Hywan Apr 16, 2015

Author Member

Ok to optimize it but it's different of blankline_after_open_tag. For instance, if we have:

<?php /*

the blankline_after_open_tag will do nothing as far as I know, or at least, it will transform it into:

<?php
/*

and we expect

<?php

/*

Any idea how to not “corrupt” the token collection? I don't even know why it corrupts? Because of the insertAt?

This comment has been minimized.

Copy link
@stof

stof Apr 16, 2015

you are confusing newline_after_open_tag and blankline_after_open_tag. blankline_after_open_tag does what you expect. newline_after_open_tag indeed enforces only having a newline, but does not force the empty line after it.

This comment has been minimized.

Copy link
@stof

stof Apr 16, 2015

and the issue about your code is not that it is not optimized. the issue is that it does not set the open tag token to the same content than the one generated by PHP when parsing the file, which could create issues with other fixers expecting to have a token collection in a consistent state (the optimizations performed by the PHP-CS-Fixer mean that the same collection instance will be reused)

This comment has been minimized.

Copy link
@Hywan

Hywan Apr 17, 2015

Author Member

blankline_after_open_tag does not work on my given example. Try to run it on:

<?php /*

or

<?php
/*

or even

<?php



/*

It won't change anything :-/.

This comment has been minimized.

Copy link
@Hywan

Hywan Apr 17, 2015

Author Member

It works in the 1.7 version.

/**
* Remove `@access`.
*/
class PhpdocAccess extends AbstractFixer

This comment has been minimized.

Copy link
@stof

stof Apr 16, 2015

Just use phpdoc_no_access instead of reimplementing it

This comment has been minimized.

Copy link
@Hywan

Hywan Apr 16, 2015

Author Member

You're right. Didn't see it!

This comment has been minimized.

Copy link
@Hywan

Hywan Apr 17, 2015

Author Member

Availabe since few days, in the 1.7. Thanks.

Hywan added 5 commits Apr 17, 2015
Fixer `blankline_after_open_tag` and `phpdoc_access` have been
introduced recently, use them since they are better maintained and
tested.

Thanks @stof for the review!
@Bhoat Bhoat merged commit 9a0d361 into hoaproject:master Apr 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.