Apply php-cs-fixer #112

bigfoot90 · 2017-01-03T00:04:12Z

Apply php-cs-fixer
https://github.com/FriendsOfPHP/PHP-CS-Fixer

codecov-io · 2017-01-03T00:07:20Z

Current coverage is 99.70% (diff: 100%)

Merging #112 into master will decrease coverage by 0.21%

@@             master       #112   diff @@
==========================================
  Files            53         53          
  Lines          3770       3777     +7   
  Methods         184        184          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           3767       3766     -1   
- Misses            3         11     +8   
  Partials          0          0

Powered by Codecov. Last update 082a1fa...165e678

ibennetch · 2017-01-03T03:06:20Z

.gitignore

@@ -1,3 +1,5 @@
+.php_cs.cache
+cs_fixer_tmp_*


I'm not terribly inclined to add these to .gitignore, but I'll wait to see what the others think about it.

I've installed php-cs-fixer as global vendor, normally I take this configuration in my global .gitignore,
but I thought for the other people who can run the tool, they can not have this configuration.

I've seen also, that you have .idea in your .gitignore, this line should be put in your global .gitignore configuration since .idea isn't realted to the project but to the ide used by programmer which can be different from person to person.

ibennetch · 2017-01-03T03:08:12Z

src/Component.php

@@ -7,9 +7,8 @@
 * There is a small difference between *Component and *Keyword classes: usually,
 * *Component parsers can be reused in multiple  situations and *Keyword parsers
 * count on the *Component classes to do their job.
- *
- * @package SqlParser


I have to admit I'm a little stumped by this, I've looked at several styleguides and none of them mention this. The phpdoc documentation still mentions it, so it doesn't seem to be deprecated. I'm just confused about it.

I think it makes less sense since namespaces were introduced, so it's probably okay to remove.

ibennetch · 2017-01-03T03:11:48Z

src/Component.php

-     * @param Parser     $parser  The parser that serves as context.
-     * @param TokensList $list    The list of tokens that are being parsed.
-     * @param array      $options Parameters for parsing.
+     * @param Parser     $parser  the parser that serves as context


This also doesn't quite make sense, and conflicts with the samples provided here. For me, it's much more readable to have the sentence (or phrase) start with a capital letter. The trailing period, I'm rather indifferent about; ideally I don't mind seeing it go away since these are often only phrases anyway.

IMHO it looks better in the documentation as sentences, but I'm quite okay with both choices.

This behavior is configurable

ibennetch · 2017-01-03T03:15:24Z

src/Components/AlterOperation.php

-        'UPGRADE'                   => array(1, 'var'),
-        'COLLATE'                   => array(2, 'var'),
-        'DEFAULT COLLATE'           => array(2, 'var'),
+        'CHARACTER SET' => array(1, 'var'),


I tend to prefer having everything lined up, but I'm not strictly tied to that.

On the other side the lined up parts are harder to maintain (you need to reindent whole block when adding something longer).

ibennetch · 2017-01-03T03:17:33Z

src/Components/AlterOperation.php

    );

    /**
-     * All view options
+     * All view options.


Now the tool seems to be making things up; earlier it was removing periods after full sentences and now it's adding them to a few words. I'm not sure what to make of this.

Probably it's a sentence for it as it starts with capital letter :-)

PhpDoc makes different summary and description sections. https://www.phpdoc.org/docs/latest/guides/docblocks.html#summary
Summay The summary is a short but effective overview of an element.
Description A description can be a long text with an elaborate explanation what the associated element does. The description is optional.

As I read, the texts after @var and @param are descriptions
https://www.phpdoc.org/docs/latest/references/phpdoc/tags/var.html#examples

ibennetch · 2017-01-03T03:17:48Z

src/Components/AlterOperation.php


        /**
         * Counts brackets.
         *
-         * @var int $brackets
+         * @var int


This seems wrong to me; I don't think it should be removing the variable name from the docblock.

The name is required only if you refer
https://www.phpdoc.org/docs/latest/references/phpdoc/tags/var.html

In this case the name of the property MAY be omitted

ibennetch · 2017-01-03T03:20:40Z

src/Components/AlterOperation.php

     *
     * @return string
     */
    public static function build($component, array $options = array())
    {
-        $ret = $component->options . ' ';
+        $ret = $component->options.' ';


Looking at some comments on PSR-1 (not the standard itself, which doesn't seem to say anything), this seems opposite the intentions of the PSR. In all the discussion and examples around it, they use the spaces on both sides. I think the spaces make it much more readable, so I'd vote to keep the spaces, too.

I also prefer to have spaces around docs.

This behavior is configurable

ibennetch · 2017-01-03T03:21:05Z

src/Components/AlterOperation.php

     *
     * @return AlterOperation
     */
    public static function parse(Parser $parser, TokensList $list, array $options = array())
    {
-        $ret = new AlterOperation();
+        $ret = new self();


This is an interesting change.

ibennetch · 2017-01-03T03:21:54Z

src/Components/CaseExpression.php

     *
     * @return Expression
     */
    public static function parse(Parser $parser, TokensList $list, array $options = array())
    {
-        $ret = new CaseExpression();
+        $ret = new self();


This is also interesting.

This will do the same

ibennetch · 2017-01-03T03:27:32Z

Hi @bigfoot90 and thanks for your submission. Some of the changes definitely look good, and some of them are not things I personally would change. I'd like to see some discussion from the rest of the team on these.

For phpMyAdmin itself, we use our own hybrid (and loose) styleguide which mostly based on the PEAR styles. It might be good to enhance this and particularly to update it (for instance, PSR now makes recommendations about namespaces that weren't possible a few years ago).

See phpmyadmin/sql-parser#112 Signed-off-by: Michal Čihař <michal@cihar.com>

nijel · 2017-01-03T10:06:46Z

Anyway I think we should first agree on coding style and then enforce it, not the other way around :-).

However I find this really useful, I've added it to motranslator and shapefile libs (with minor adjustments such as whitespace around . operator) right now as the outcome is less problematic there:

phpmyadmin/motranslator@cf71d7c (configuration)
phpmyadmin/motranslator@d29162c (fixes)
phpmyadmin/shapefile@ec6191f (configuration)
phpmyadmin/shapefile@43b86b7 (fixes)

bigfoot90 · 2017-01-03T19:45:15Z

src/Components/AlterOperation.php

@@ -17,86 +15,84 @@
 * Parses an alter operation.


Duplicated comment, see before namespace

bigfoot90 · 2017-01-03T21:19:06Z

Sorry, closed this wrong. My mistake.
Why there are 0 commints and 0 file changed?

Please read my comments anyway.

bigfoot90 force-pushed the php-cs-fixer branch 2 times, most recently from 2193f70 to 165e678 Compare January 3, 2017 00:10

ibennetch reviewed Jan 3, 2017

View reviewed changes

nijel added a commit to phpmyadmin/motranslator that referenced this pull request Jan 3, 2017

Add phpcs configuration

cf71d7c

See phpmyadmin/sql-parser#112 Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to phpmyadmin/shapefile that referenced this pull request Jan 3, 2017

Add phpcs configuration

ec6191f

See phpmyadmin/sql-parser#112 Signed-off-by: Michal Čihař <michal@cihar.com>

bigfoot90 commented Jan 3, 2017

View reviewed changes

bigfoot90 closed this Jan 3, 2017

bigfoot90 force-pushed the php-cs-fixer branch from 19668cf to 4f5cac4 Compare January 3, 2017 20:56

bigfoot90 deleted the php-cs-fixer branch January 3, 2017 21:03

bigfoot90 restored the php-cs-fixer branch January 3, 2017 21:04

bigfoot90 mentioned this pull request Jan 3, 2017

Apply php-cs-fixer #114

Merged

		@@ -1,3 +1,5 @@
		.php_cs.cache
		cs_fixer_tmp_*

Uh oh!

Apply php-cs-fixer #112

Apply php-cs-fixer #112

Uh oh!

Conversation

bigfoot90 commented Jan 3, 2017

Uh oh!

codecov-io commented Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 99.70% (diff: 100%)

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nijel Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibennetch commented Jan 3, 2017

Uh oh!

nijel commented Jan 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigfoot90 commented Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Jan 3, 2017 •

edited

Loading

nijel Jan 3, 2017 •

edited

Loading

bigfoot90 commented Jan 3, 2017 •

edited

Loading