Add PHPDoc blocks to AbstractMigration #1052

koninka · 2020-08-31T06:18:24Z

Q	A
Type	improvement
BC Break	no
Fixed issues

Summary

Add PHPDoc blocks to AbstractMigration methods. This could be useful for those who use @inheritdocannotation (like me:)).

greg0ire · 2020-08-31T07:06:29Z

Why would it be useful?

koninka · 2020-08-31T07:26:50Z

@greg0ire

Hey,

It's good practice to document your code.
Having a description for the methods makes it easy to understand what they do. For example, the write and warnIf methods are not immediately obvious without additional code reading (not only in the AbstractMigration class).
My case. I always document my code and use @inheritdoc for overridden vendor methods (e.g. getDescription). And if I use @inheritdoc and parent method has no phpdocs, then this is at least strange (also PHPStorm reports warning).

You have a description of the methods on the site, why not place the documentation in the code to save programmers from the need to google or research the code in depth to understand its work?

greg0ire · 2020-08-31T09:28:15Z

It is, but documentation has to bring value, otherwise you risk people not reading useful comment because most comments in your project are just noise. I'd argue that's the case of most comments in this PR.
That's the part of the PR that might bring some value. If I were to merge anything here, that would be it.
That's really personal, sorry but we are not going to pollute the code with tautological comments. If you want to add comments, they should bring value.

You have a description of the methods on the site, why not place the documentation in the code to save programmers from the need to google or research the code in depth to understand its work?

That's a good point, maybe there is valuable information here that could be fit in the comments. Description of this migration is not valuable information IMO. So is information already contained in type declarations.

koninka · 2020-08-31T09:39:54Z

@greg0ire

I got it. So, do I understand correctly, if I add a more useful description for the getDescription, then you will merge pr? Do I need to add a more detailed description to other methods?

P.S. I took a description of all methods from the official site (for method getDescription too).

greg0ire · 2020-08-31T12:16:52Z

lib/Doctrine/Migrations/AbstractMigration.php

@@ -60,11 +60,22 @@ public function isTransactional() : bool
        return true;
    }

+    /**
+     * Description of this migration.


Tautological, either improve or remove

greg0ire · 2020-08-31T12:17:28Z

lib/Doctrine/Migrations/AbstractMigration.php

+     *
+     * @return string


Nothing new here, let's remove it

Suggested change

*

* @return string

greg0ire · 2020-08-31T12:17:40Z

lib/Doctrine/Migrations/AbstractMigration.php

+     *
+     * @param bool   $condition
+     * @param string $message


Suggested change

*

* @param bool $condition

* @param string $message

greg0ire · 2020-08-31T12:18:15Z

lib/Doctrine/Migrations/AbstractMigration.php

    public function getDescription() : string
    {
        return '';
    }

+    /**
+     * Warn with a message if some condition is met.


That's made fairly clear just by reading the signature (you don't have to read the code inside the method)

greg0ire · 2020-08-31T12:18:43Z

lib/Doctrine/Migrations/AbstractMigration.php

@@ -155,6 +166,11 @@ public function getSql() : array
        return $this->plannedSql;
    }

+    /**
+     * Write some debug information to the console.


debug information is written with debug(), this is a notice.

greg0ire · 2020-08-31T12:18:53Z

lib/Doctrine/Migrations/AbstractMigration.php

+     *
+     * @param string $message


Suggested change

*

* @param string $message

greg0ire · 2020-08-31T12:19:06Z

phpcs.xml.dist

+    <rule ref="SlevomatCodingStandard.TypeHints.ReturnTypeHint.UselessAnnotation">
+        <exclude-pattern>lib/Doctrine/Migrations/AbstractMigration.php</exclude-pattern>
+    </rule>
+


Please revert this.

greg0ire · 2020-08-31T12:32:01Z

So, do I understand correctly, if I add a more useful description for the getDescription, then you will merge pr?

It's the opposite actually, if we merge the PR, then it means you will have added a more useful description for that method, but that will probably not be enough, since other methods need a better description too.

greg0ire · 2020-09-04T05:55:14Z

lib/Doctrine/Migrations/AbstractMigration.php

+    /**
+     * Description of this migration.
+     *
+     * Describe what this migration does in simple terms. This information is displayed when you view the list of migrations.
+     */


Suggested change

/**

* Description of this migration.

*

* Describe what this migration does in simple terms. This information is displayed when you view the list of migrations.

*/

/**

* Describe what this migration does in simple terms. This information is displayed when you view the list of migrations.

*/

greg0ire · 2020-09-04T05:56:39Z

lib/Doctrine/Migrations/AbstractMigration.php

+    /**
+     * Write some debug information to the console.
+     *
+     * Debug information is written with debug().


🤔 I mean it should, but it's actually written with notice(), so that comment is just misleading

koninka force-pushed the add-phpdoc-for-getdescription-method branch from cfbd10f to b45758d Compare August 31, 2020 06:30

greg0ire requested changes Aug 31, 2020

View reviewed changes

Add PHPDoc blocks to AbstractMigration

62f6570

koninka force-pushed the add-phpdoc-for-getdescription-method branch from b45758d to 62f6570 Compare September 4, 2020 03:41

greg0ire reviewed Sep 4, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PHPDoc blocks to AbstractMigration #1052

Add PHPDoc blocks to AbstractMigration #1052

koninka commented Aug 31, 2020

greg0ire commented Aug 31, 2020

koninka commented Aug 31, 2020

greg0ire commented Aug 31, 2020

koninka commented Aug 31, 2020

greg0ire Aug 31, 2020

koninka Sep 4, 2020

greg0ire Aug 31, 2020

koninka Sep 4, 2020

greg0ire Aug 31, 2020

koninka Sep 4, 2020

greg0ire Aug 31, 2020

koninka Sep 4, 2020

greg0ire Aug 31, 2020

koninka Sep 4, 2020

greg0ire Aug 31, 2020

koninka Sep 4, 2020

greg0ire Aug 31, 2020

koninka Sep 4, 2020

greg0ire commented Aug 31, 2020

greg0ire Sep 4, 2020

greg0ire Sep 4, 2020

Add PHPDoc blocks to AbstractMigration #1052

Are you sure you want to change the base?

Add PHPDoc blocks to AbstractMigration #1052

Conversation

koninka commented Aug 31, 2020

Summary

greg0ire commented Aug 31, 2020

koninka commented Aug 31, 2020

greg0ire commented Aug 31, 2020

koninka commented Aug 31, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Aug 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment