Make code-style more consistent #355

alcohol · 2017-02-16T11:33:56Z

..through the use of a php-cs-fixer configuration and applying it.

Since there were a lot of inconsistencies already present in the codebase, I was not sure which rules would be preferred in some access (e.g. align double arrows in array notation, etc).

Putting this PR up here for feedback and discussion.

bighappyface · 2017-02-16T12:23:41Z

composer.json

@@ -52,12 +52,12 @@
    "bin": ["bin/validate-json"],
    "extra": {
        "branch-alias": {
-            "dev-master": "4.0.x-dev"
+            "dev-master": "5.0.x-dev"


bighappyface · 2017-02-16T12:27:57Z

.php_cs.dist

+$config
+    ->setRules([
+        // default
+        '@PSR2' => true,


I am good with PHPCS fixer being run at large to get the baseline style into the repo, but may we have PHPCS evaluation added to the TravisCI config to catch infractions moving forward?

Thanks a lot for this contribution. I have been thinking about this for awhile but never got around to it.

Is there a specific need or requirement that phpcodesniffer solves that php-cs-fixer does not? If Travis integration is the only thing, then I would like to point out that php-cs-fixer could just as easily be integrated into the Travis process. I just did not do it right away as not everyone is always in favor of this. E.g. at @composer we usually let style issues slip through in PR's because we do not want those to be a barrier for contributors. We run php-cs-fixer occasionally over the codebase ourselves manually.

Or maybe I misunderstood and you were not referring to phpcodesniffer (which is generally called phpcs) but rather php-cs-fixer? If that is the case, I'll add it to the Travis flow. Any feedback on the ruleset? Removals, additions?

Sorry for the confusion. I am cool with using php-cs-fixer to evaluate code style and am not tied to phpcs specifically. Adding to the TravisCI flow will be very helpful.

I am fine with the ruleset but am curious if the additional configs override/supplement defaults for the PSR-2 spec or the Symfony spec?

Mostly Symfony overrides and some contrib rules (not part of Symfony nor any PSR).

The array syntax is because of the PHP supported version.

The concat with space seemed the most prevalent choice already in the codebase.

No unused imports I generally turn off since the detection does not work well with use statements that are only referenced in docblocks (at least that used to be my experience).

No useless return and ordered imports are part of contrib.

PHP doc align is a part of Symfony but didn't seem the most prevalent approach in current codebase.

PHP doc no package is part of Symfony and removes all @Package docblocks. Not sure if you wanted to keep these so I did not remove them for now.

Trailing comma in array is part of Symfony was not yet the most prevalent approach so I excluded it.

Unfortunately php-cs-fixer does not play nice together with HHVM. Since HHVM is one of the required versions in the matrix that must pass, I have to resort to dynamically installing php-cs-fixer. I opted to only install it on one specific build (chose the same one that is used for codecoverage, but can also move it to 7.1 if preferred).

bighappyface · 2017-02-16T15:05:28Z

@shmax @erayd thoughts before this is merged and other PRs are subject to code style analysis?

shmax · 2017-02-16T17:02:46Z

~~I have no objections. LGTM.~~

Hah, guess I should have actually looked at the changes. Retracting my LGTM!

erayd · 2017-02-16T20:27:30Z

@bighappyface My thoughts generally:

I really like the idea of having code style be a travis requirement to pass. It's good for the quality of submissions generally.
I do not like the idea of manually running the fixer on the codebase from time-to-time - it makes git blame annoying, because it'll flag the style commit rather than who actually changed stuff unless you start stepping back through the history. IMO much better to have the code be styled correctly in the first place before it's merged.
I like the choice of PSR2 as a style.
Quite happy to have an initial bulk run of the fixer over the current codebase to get things consistent. It'll wreck git blame, but I don't think there's any sensible way to avoid this, so let's just do it once.
I don't care which style validator / fixer is used, as long as it's reliable. I'm more familiar with PHPCS, but anything that does the job is fine with me.
To make contributions a bit smoother, having a 'fix-style' or similar composer script seems like a good idea. That way contributors have an easy way to force their own code to be compliant before submitting it.

erayd · 2017-02-16T20:29:08Z

src/JsonSchema/Constraints/Constraint.php

-    const CHECK_MODE_COERCE_TYPES =     0x00000004;
-    const CHECK_MODE_APPLY_DEFAULTS =   0x00000008;
-    const CHECK_MODE_EXCEPTIONS =       0x00000010;
+    const CHECK_MODE_NONE = 0x00000000;


Can we have a rule that allows / enforces alignment of constants like this, rather than removing the whitespace?

I'd rather have enforced style than aligned constants, but both would be nice :-).

@alcohol are there comment delimiters or other facility to skip code blocks for php-cs-fixer?

Not to my knowledge, but there is a whitespace rule to align equal signs.

erayd · 2017-02-16T20:36:09Z

src/JsonSchema/Constraints/CollectionConstraint.php

@@ -136,7 +137,7 @@ protected function validateItems(&$value, $schema = null, JsonPointer $path = nu

            // Treat when we have more schema definitions than values, not for empty arrays
            if (count($value) > 0) {
-                for ($k = count($value); $k < count($schema->items); $k++) {
+                for ($k = count($value); $k < count($schema->items); ++$k) {


This seems dangerous if the auto-fixer made this change; they aren't syntactically equivalent. It doesn't matter in this particular case, but if the same change is made in a place that relies on the return value of ++, then it'll alter the logic.

$var++; //increments $var and returns the pre-increment value
++$var; // increments $var and returns the post-increment value

I know, it is considered a risky fixer. They can be disabled explicitly.

Could we please disable this one then?

Yes, let me revert/rebase the changes and re-apply them.

erayd · 2017-02-16T20:49:39Z

src/JsonSchema/Uri/UriResolver.php

     */
    public static function combineRelativePathWithBasePath($relativePath, $basePath)
    {
        $relativePath = self::normalizePath($relativePath);
        if ($relativePath == '') {
            return $basePath;
        }
-        if ($relativePath{0} == '/') {
+        if ($relativePath[0] == '/') {


Is this intended to be $relativePath[0], or $relativePath->{'0'}? Both options are a possibility; dynamic object fields are used elsewhere in the code. Is $relativePath always an array (or an object that implements array access), or can it sometimes be a non-array-compliant object?

As far as I know, it is a string and array access is merely used as a quick way to check the first character.

This is just a string. The curly and square brace styles are equivalent, but most people are more likely to be familiar with the square bracket notation, so it's probably the best way to go.

Sweet - should be fine then :-). Thanks for clarifying.

erayd · 2017-02-16T20:56:19Z

tests/Constraints/FactoryTest.php

-      return array(
+    public function invalidConstraintNameProvider()
+    {
+        return array(


The indenting went from OK to broken here. [ed: the broken indenting is not included in GitHub's highlighted lines for this comment; need to view the code just below this as well to see the problem.]

erayd · 2017-02-16T21:00:39Z

tests/Constraints/FactoryTest.php

        array('invalidConstraintName'),
      );
    }

    /**
-     * @expectedException InvalidArgumentException
+     * @expectedException \InvalidArgumentException


This is probably incorrect - \JsonSchema\Exception\InvalidArgumentException is a thing.

But that is not the current namespace?

It's not the current namespace, but it's probably what was intended, as convention everywhere else in the project is to use the namespaced exceptions. This one is less "the auto-fixer screwed up", and more "the original source screwed up, and the auto-fixer perpetuated that, so we should fix now it rather than making the error worse".

Sure I can change it, and check if the tests still pass. Give me a few minutes.

erayd · 2017-02-16T21:01:29Z

tests/Constraints/FactoryTest.php

    }

    /**
-     * @expectedException InvalidArgumentException
+     * @expectedException \InvalidArgumentException


As per above, this probably isn't intended to be a root-namespace exception.

erayd · 2017-02-16T21:02:42Z

tests/Constraints/OfPropertiesTest.php

-                        "pointer"    => "/prop2",
-                        "message"    => "Array value found, but a string is required",
-                        "constraint" => "type",
+                        'property' => 'prop2',


Is there any option for an alignment rule for this kind of thing? Not a huge problem if there is not, but it would be nice to keep things neat.

Yes there is, but since the majority of the codebase did not align the double arrow in array notations, I opted to make this consistent everywhere. If however it is desired that these be aligned, then the rule can simply be changed to that and the rest of the code will be adjusted accordingly.

That's a fair call. @bighappyface & @shmax - what are your thoughts on enabling this globally?

@alcohol - If global is undesirable for some reason, would it be possible to enable it just for the testsuite? That's where most of the array stuff that needs aligning is located anyway.

alcohol · 2017-02-17T08:44:13Z

The alignment rules make things more consistent but unfortunately also look worse in some cases. Perhaps you would prefer to just turn them off (both, as in don't align and don't un-align)?

shmax · 2017-02-17T09:15:51Z

I guess I could go either way on the object alignment, but I've never seen the other kind of inline alignment happening here and there in the code before, so my instinct is to shy away from it--it gives the code a kind of jittery feel, and it seems like the sort of thing that might be tough to second-guess as one is styling by hand. I guess my vote is to not do it, as @alcohol says.

bighappyface · 2017-02-17T14:10:47Z

I am fine to skip alignment fixes

alcohol · 2017-02-17T14:47:41Z

Updated PR accordingly.

erayd

Looks good! Thanks @alcohol :-).

update branch-alias

3e30483

bighappyface requested changes Feb 16, 2017

View reviewed changes

bighappyface approved these changes Feb 16, 2017

View reviewed changes

bighappyface added the Pending Merge label Feb 16, 2017

erayd reviewed Feb 16, 2017

View reviewed changes

bighappyface mentioned this pull request Feb 16, 2017

Add option to apply default values from the schema #349

Merged

alcohol added 3 commits February 17, 2017 15:34

add php-cs-fixer configuration

1e6d0bd

add php-cs-fixer to travis

03369eb

apply code-style rules

9dc92a2

erayd approved these changes Feb 17, 2017

View reviewed changes

bighappyface merged commit 42c1043 into justinrainbow:master Feb 21, 2017

Make code-style more consistent #355

Make code-style more consistent #355

Conversation

alcohol commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcohol Feb 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcohol Feb 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bighappyface commented Feb 16, 2017

shmax commented Feb 16, 2017 • edited

erayd commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erayd Feb 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erayd Feb 17, 2017 • edited

Choose a reason for hiding this comment

alcohol commented Feb 17, 2017

shmax commented Feb 17, 2017

bighappyface commented Feb 17, 2017

alcohol commented Feb 17, 2017

erayd left a comment

Choose a reason for hiding this comment

alcohol Feb 16, 2017 •

edited

alcohol Feb 16, 2017 •

edited

shmax commented Feb 16, 2017 •

edited

erayd Feb 16, 2017 •

edited

erayd Feb 17, 2017 •

edited