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

Adopt PSR-12 as base coding standard #148

Merged
merged 2 commits into from
Jan 16, 2020
Merged

Adopt PSR-12 as base coding standard #148

merged 2 commits into from
Jan 16, 2020

Conversation

carusogabriel
Copy link
Contributor

@carusogabriel carusogabriel commented Dec 27, 2019

This PR proposes that the Doctrine's CS adopts PSR-12 as the base for our standards, with a few changes, as voted in the past.

Important changes:

tests/input/trailing_comma_on_array.php Show resolved Hide resolved
@carusogabriel
Copy link
Contributor Author

As per item 5.1 of the PSR:

Expressions in parentheses MAY be split across multiple lines, where each subsequent line is indented at least once. When doing so, the first condition MUST be on the next line. The closing parenthesis and opening brace MUST be placed together on their own line with one space between them. Boolean operators between conditions MUST always be at the beginning or at the end of the line, not a mix of both.

I've found some occurrences in doctrine/migrations:

diff --git a/lib/Doctrine/Migrations/Generator/SqlGenerator.php b/lib/Doctrine/Migrations/Generator/SqlGenerator.php
index 8c92c12..9868a28 100644
--- a/lib/Doctrine/Migrations/Generator/SqlGenerator.php
+++ b/lib/Doctrine/Migrations/Generator/SqlGenerator.php
@@ -47,8 +47,10 @@ class SqlGenerator

         $storageConfiguration = $this->configuration->getMetadataStorageConfiguration();
         foreach ($sql as $query) {
-            if ($storageConfiguration instanceof TableMetadataStorageConfiguration
-                && stripos($query, $storageConfiguration->getTableName()) !== false) {
+            if (
+                $storageConfiguration instanceof TableMetadataStorageConfiguration
+                && stripos($query, $storageConfiguration->getTableName()) !== false
+            ) {
                 continue;
             }

and

diff --git a/lib/Doctrine/Migrations/Provider/LazySchemaDiffProvider.php b/lib/Doctrine/Migrations/Provider/LazySchemaDiffProvider.php
index 0d0df44..3606bd2 100644
--- a/lib/Doctrine/Migrations/Provider/LazySchemaDiffProvider.php
+++ b/lib/Doctrine/Migrations/Provider/LazySchemaDiffProvider.php
@@ -81,8 +81,10 @@ class LazySchemaDiffProvider implements SchemaDiffProvider
     /** @return string[] */
     public function getSqlDiffToMigrate(Schema $fromSchema, Schema $toSchema) : array
     {
-        if ($toSchema instanceof LazyLoadingInterface
-            && ! $toSchema->isProxyInitialized()) {
+        if (
+            $toSchema instanceof LazyLoadingInterface
+            && ! $toSchema->isProxyInitialized()
+        ) {
             return [];
         }

Is it something that we want? 👍or 👎

malarzm
malarzm previously approved these changes Dec 28, 2019
greg0ire
greg0ire previously approved these changes Dec 29, 2019
morozov
morozov previously approved these changes Dec 29, 2019
Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carusogabriel would you please add a test for the multiline thing (and to any other sniff we're implicitly adding here)?

@carusogabriel
Copy link
Contributor Author

@lcobucci Yes sir :)

I'll add for the following ones that I've found:

Also, as already reported in squizlabs/PHP_CodeSniffer#460, looks like we need to exclude PSR2.ControlStructures.ControlStructureSpacing.SpacingAfterOpenBrace in favor os PSR-12 (!?)

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a minor suggestion, rest LGTM once tests have been added 🎉

lib/Doctrine/ruleset.xml Show resolved Hide resolved
alcaeus
alcaeus previously approved these changes Jan 16, 2020
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, Gabriel!

malarzm
malarzm previously approved these changes Jan 16, 2020
alcaeus
alcaeus previously approved these changes Jan 16, 2020
@alcaeus
Copy link
Member

alcaeus commented Jan 16, 2020

@carusogabriel might want to revisit those build failures though ;)

@carusogabriel
Copy link
Contributor Author

carusogabriel commented Jan 16, 2020

@alcaeus Yeah, I'm trying to figure the make update-compatibility-patch out :(

Copy link
Member

@deeky666 deeky666 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, thanks

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Copy link
Member

@malarzm malarzm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍾

@alcaeus alcaeus dismissed stale reviews from lcobucci and Ocramius January 16, 2020 13:35

PR udpated

@alcaeus
Copy link
Member

alcaeus commented Jan 16, 2020

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants