Switch from StyleCI to PHP-CS fixer #502

crissi · 2019-10-24T17:48:32Z

now developers can run composer fix to fix styling issues and more easily set up their IDE because we are using phpcsfixer

and travis will fail and show the error diff from php-cs-fixer

TODOs

Don't know how to disable styleci
I am running the linter on every environment in travis, this could probably be changed for faster a pipeline. Don't know how easy that is!?

Links

Connected to Should the project switch to a different code / style / enforcer #497

mfn · 2019-10-24T19:30:47Z

I am running the linter on every environment in travis, this could probably be changed for faster a pipeline. Don't know how easy that is!?

Done, pushed a commit! I just use env variables for this and we just need to make sure that at least for one job it's set. Same approach used for e.g. phpstan.

I also fixed the failing lumen integration test. I boy I hate them, but it's better then nothing. They rely on scripting the manual steps necessary and easily fail due to the sensitivity of the regex when the source file changes even formatting (which happened in this case for config/config.php). Anyway, all green!

I also took the liberty to simply remove the extended tests from 5.8; it's EOL anyway and only wil receive security fixes which shouldn't be critical for the extended test.

Don't know how to disable styleci

Only admins can do that (which I'm not).

But before proceeding I would like to do some testing/feedbacking

mfn

Thanks for the initiative, that's really great; love it so far!

I think the hard part is now on agreeing on the actual config, see my inline thoughts on some of this.

My perfect outcome would be the most minimal configuration with the most aggressive approach so that no room for interpretation is left.

(ps: noticed that PhpStorm detects php-cs fixer and can auto correct it: how awesome is that)

mfn · 2019-10-24T19:34:08Z

.php_cs.dist

+<?php
+
+/**
+ * Rules we follow are from PSR-2 as well as the rectified PSR-2 guide.


Is there a blueprint where you got this whole fle from?

https://gist.github.com/codfish/c77d348820c1c6b4ebe4a66dc2291c74#file-php_cs-dist

mfn · 2019-10-24T19:39:29Z

.php_cs.dist

+
+return PhpCsFixer\Config::create()
+    ->setRules([
+        '@PSR2' => true,


So. I was wondering since it said @PSR2 at the top, why the other rules?

I removed them all and ran fix and it did quite some changes, like these:

diff --git a/src/GraphQL.php b/src/GraphQL.php index c5d3b0d..9562554 100644 --- a/src/GraphQL.php +++ b/src/GraphQL.php @@ -239,10 +239,12 @@ class GraphQL if (! $type instanceof TypeConvertible) { throw new TypeNotFound( - sprintf('Unable to convert %s to a GraphQL type, please add/implement the interface %s', + sprintf( + 'Unable to convert %s to a GraphQL type, please add/implement the interface %s', get_class($type), TypeConvertible::class - )); + ) + ); } foreach ($opts as $key => $value) { diff --git a/src/Support/ResolveInfoFieldsAndArguments.php b/src/Support/ResolveInfoFieldsAndArguments.php index 34aceea..bed6194 100644 --- a/src/Support/ResolveInfoFieldsAndArguments.php +++ b/src/Support/ResolveInfoFieldsAndArguments.php @@ -130,12 +130,16 @@ class ResolveInfoFieldsAndArguments $spreadName = $selectionNode->name->value; if (isset($this->info->fragments[$spreadName])) { $fragment = $this->info->fragments[$spreadName]; - $fields = array_merge_recursive($this->foldSelectionSet($fragment->selectionSet, $descend), - $fields); + $fields = array_merge_recursive( + $this->foldSelectionSet($fragment->selectionSet, $descend), + $fields + ); } } elseif ($selectionNode instanceof InlineFragmentNode) { - $fields = array_merge_recursive($this->foldSelectionSet($selectionNode->selectionSet, $descend), - $fields); + $fields = array_merge_recursive( + $this->foldSelectionSet($selectionNode->selectionSet, $descend), + $fields + ); } }

or

diff --git a/tests/Database/MutationValidationUniqueWithCustomRulesTests/MutationValidationUniqueWithCustomRulesTest.php b/tests/Database/MutationValidationUniqueWithCustomRulesTests/MutationValidationUniqueWithCustomRulesTest.php index 82a2932..e1a7581 100644 --- a/tests/Database/MutationValidationUniqueWithCustomRulesTests/MutationValidationUniqueWithCustomRulesTest.php +++ b/tests/Database/MutationValidationUniqueWithCustomRulesTests/MutationValidationUniqueWithCustomRulesTest.php @@ -36,7 +36,8 @@ GRAPHQL; ], ]); - $this->assertSqlQueries(<<<'SQL' + $this->assertSqlQueries( + <<<'SQL'

Basically it felt more aggressive.

I liked that because for example, in the current version these two patterns are allowed:

$this->assertSqlQueries( <<<'SQL'

and

$this->assertSqlQueries(<<<'SQL'

Nether one is touched and converted to the other.

Yet when I remove all the settings, it's converted the former form always.

I think this is better, the source will be more unified and leaves less room for interpretation.

mfn · 2019-10-24T19:40:36Z

.php_cs.dist

+return PhpCsFixer\Config::create()
+    ->setRules([
+        '@PSR2' => true,
+        'array_syntax' => [ 'syntax' => 'short' ],


Immediately noticed though, per my above comment, removing this one here means it will not enforce $foo = array(); being converted to $foo = [];

So I guess just @PSR2 isn't specific enough and still has room for interpretation.

yeah @psr2 is not enough to get the "prettiest" formatting:-)

mfn

I've found https://gist.github.com/laravel-shift/cab527923ed2a109dda047b97d53c200 and applied and pushed the changes for review.

Not sold yet on the config though :)

I wish there was a better way then copypasting some gist around; maybe there is a package with a maintained "Laravel" compatible PHP-CS fixer config we could just be pulled it as a dev dependency?

mfn · 2019-11-05T21:24:28Z

src/Support/Field.php

@@ -259,7 +259,7 @@ public function __get($key)
    {
        $attributes = $this->getAttributes();

-        return isset($attributes[$key]) ? $attributes[$key] : null;
+        return $attributes[$key] ?? null;


Now this is really nice

mfn · 2019-11-05T21:25:07Z

src/Support/SelectFields.php

}
+

crissi · 2019-11-05T21:51:14Z

Maybe this? https://github.com/matt-allan/laravel-code-style

mfn · 2019-11-08T20:18:22Z

I pushed some updates:

we need to use matt-allan/laravel-code-style dev-master because alphabetical sorting of imports is fixed there but it's not yet released
- and since we already use alphabetical imports, it wouldn't make sense to go back and forth on this
- PS: I'm fine with dev-master because it's "only" a dev package
the package causes conflicts with older PHP versions we test (7.1) and older Laravel rrameworks (5.5). But since we only need this be running for one job at least, I simply made it to be uninstalled for all parts we don't run the linter

mfn · 2019-11-08T20:30:00Z

Also added a PR template which mentions how to fix the code style

mfn

Thanks @crissi , that's some great contribution here: I'm a sucker for code style standardization 😄

From my side this is 👍 good to go and now we need to remove StyleCI

@rebing TL;DR: we propose to switch from StyleCI to PHP-CS fixer which is fully integrated in the pipeline and has the benefit that contributors can fix code style violations locally. If that's good with you, could you please remove the StyleCI integration completely here? 🙏

EdwinDayot · 2019-11-12T15:35:24Z

+1 for this ! Being able to fix everything locally is awesome. It would be great being able to do the same thing for travis, but I can imagine what a mess it could be.

johannesnagl · 2019-11-12T19:42:35Z

I've added a small fix in here: crissi#9

mfn · 2019-11-12T20:16:18Z

@johannesnagl

I've added a small fix in here: crissi#9

Thanks, already cherry-picked plus some other fixes!

crissi · 2019-11-12T23:04:07Z

So I gues we just need to fix to make it pass the styleci check and then after this PR is merged the styleci is gone?

From https://gist.github.com/laravel-shift/cab527923ed2a109dda047b97d53c200

…ha sorted imports

Otherwise composer can't install the dependencies correctly because this package has higher version constraints.

Primarily for a way to signal devs how to fix the code style, which the linter might error out on.

mfn · 2019-11-13T22:25:23Z

@rebing can you also please remove StyleCI? Thanks 🤗

mfn reviewed Oct 24, 2019

View reviewed changes

mfn force-pushed the add_php_cs_fixer branch from fce88bd to 361b4cd Compare November 5, 2019 21:20

mfn reviewed Nov 5, 2019

View reviewed changes

mfn force-pushed the add_php_cs_fixer branch from e3f0ad8 to 8c6b2d4 Compare November 8, 2019 20:08

mfn force-pushed the add_php_cs_fixer branch 2 times, most recently from f88223b to 9797ca6 Compare November 8, 2019 20:29

mfn approved these changes Nov 8, 2019

View reviewed changes

mfn changed the title ~~[WIP] add php cs fixer~~ Switch from StyleCI to PHP-CS fixer Nov 8, 2019

mfn mentioned this pull request Nov 12, 2019

Fixed the custom query not being handled by interface's relations #486

Merged

mfn mentioned this pull request Nov 12, 2019

removed styleci image from readme crissi/graphql-laravel#9

Closed

crissi and others added 12 commits November 13, 2019 20:19

add php cs fixer

040024c

remove styleci

5481ddb

git ignore cache file

c440b71

fix code

83b8207

travis: only run linting once

7be26ae

travis: fix lumen integration due to sensitive regex and source changes

c6bc0a8

travis: remove extended tests on 5.8, ~6.0 is the future

f1758b1

php-cs: apply laravel-ish rules

d8444dd

From https://gist.github.com/laravel-shift/cab527923ed2a109dda047b97d53c200

composer.phar fix

e1366d3

laravel phpcsfix config

35e1e12

Use current dev master of matt-allan/laravel-code-style so we get alp…

19a2d7b

…ha sorted imports

composer.phar fix-style

47b4def

mfn and others added 6 commits November 13, 2019 20:19

travis: remove php cs fixer package when not linting

bdc2a5a

Otherwise composer can't install the dependencies correctly because this package has higher version constraints.

Fix wrong class on formatError arg, introduced with a recent commit 🤷‍♀️

8d79934

Add github pull request template

bdfde8b

Primarily for a way to signal devs how to fix the code style, which the linter might error out on.

removed styleci image from readme

74f346d

Remove already removed styleci file from gitattributes

69bf6e6

Update changelog

0d6320c

mfn force-pushed the add_php_cs_fixer branch from e9bc260 to 0d6320c Compare November 13, 2019 19:19

mfn requested a review from rebing November 13, 2019 19:49

rebing approved these changes Nov 13, 2019

View reviewed changes

mfn merged commit 0d6320c into rebing:master Nov 14, 2019

mfn mentioned this pull request Nov 14, 2019

Should the project switch to a different code / style / enforcer #497

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from StyleCI to PHP-CS fixer #502

Switch from StyleCI to PHP-CS fixer #502

crissi commented Oct 24, 2019 •

edited

Loading

mfn commented Oct 24, 2019

mfn left a comment

mfn Oct 24, 2019

crissi Oct 24, 2019

mfn Oct 24, 2019

mfn Oct 24, 2019

crissi Oct 24, 2019

mfn left a comment

mfn Nov 5, 2019

mfn Nov 5, 2019

crissi commented Nov 5, 2019 via email •

edited

Loading

mfn commented Nov 8, 2019 •

edited

Loading

mfn commented Nov 8, 2019

mfn left a comment

EdwinDayot commented Nov 12, 2019

johannesnagl commented Nov 12, 2019

mfn commented Nov 12, 2019

crissi commented Nov 12, 2019 •

edited

Loading

mfn commented Nov 13, 2019

Switch from StyleCI to PHP-CS fixer #502

Switch from StyleCI to PHP-CS fixer #502

Conversation

crissi commented Oct 24, 2019 • edited Loading

TODOs

Links

mfn commented Oct 24, 2019

mfn left a comment

Choose a reason for hiding this comment

mfn Oct 24, 2019

Choose a reason for hiding this comment

crissi Oct 24, 2019

Choose a reason for hiding this comment

mfn Oct 24, 2019

Choose a reason for hiding this comment

mfn Oct 24, 2019

Choose a reason for hiding this comment

crissi Oct 24, 2019

Choose a reason for hiding this comment

mfn left a comment

Choose a reason for hiding this comment

mfn Nov 5, 2019

Choose a reason for hiding this comment

mfn Nov 5, 2019

Choose a reason for hiding this comment

crissi commented Nov 5, 2019 via email • edited Loading

mfn commented Nov 8, 2019 • edited Loading

mfn commented Nov 8, 2019

mfn left a comment

Choose a reason for hiding this comment

EdwinDayot commented Nov 12, 2019

johannesnagl commented Nov 12, 2019

mfn commented Nov 12, 2019

crissi commented Nov 12, 2019 • edited Loading

mfn commented Nov 13, 2019

crissi commented Oct 24, 2019 •

edited

Loading

crissi commented Nov 5, 2019 via email •

edited

Loading

mfn commented Nov 8, 2019 •

edited

Loading

crissi commented Nov 12, 2019 •

edited

Loading