As documented in the handbook. It's doxygen'd, it's tested, it's necessary. Go!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

It's doxygen'd,

  1. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/Iterator.php
    @@ -0,0 +1,56 @@
    +  /**
    +   * @param $key
    ...
    +  protected function transformKey($key, MigrateExecutable $migrate_executable, Row $row) {
    

    So nice, all the parameters are doxygen'd but what is the function actually doing?

  2. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/Iterator.php
    --- /dev/null
    +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/IteratorTest.php
    
    +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/IteratorTest.php
    @@ -0,0 +1,86 @@
    + * @file
    + * Contains
    + */
    +
    ...
    +/**
    + * @group migrate
    + * @group Drupal
    + */
    +class IteratorTest extends MigrateTestCase {
    

    It would be great to at least tell people which class is tested here.

chx’s picture

FileSize
4.84 KB
1.31 KB
jibran’s picture

Here are some more points and suggestions.

  1. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/Iterator.php
    @@ -0,0 +1,58 @@
    + */
    +
    +
    +namespace Drupal\migrate\Plugin\migrate\process;
    

    Extra space.

  2. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/Iterator.php
    @@ -0,0 +1,58 @@
    +   * Run the process pipeline for the current key.
    

    Runs

  3. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/Iterator.php
    @@ -0,0 +1,58 @@
    +   * @param $key
    

    Typehint missing.

  4. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/Iterator.php
    @@ -0,0 +1,58 @@
    +   * @param MigrateExecutable $migrate_executable
    ...
    +   * @param Row $row
    

    Fully namespaced class is required for typehint.

  5. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/Iterator.php
    @@ -0,0 +1,58 @@
    +   * @return mixed
    

    I think mix is fine but what about int|string|whatever and then in desc explain all the cases. Just a suggestion.

  6. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/IteratorTest.php
    @@ -0,0 +1,88 @@
    + * Contains
    

    Incomplete.

  7. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/IteratorTest.php
    @@ -0,0 +1,88 @@
    + * Test the iterator process plugin.
    ...
    +   * Test the iterator process plugin.
    

    Tests

  8. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/IteratorTest.php
    @@ -0,0 +1,88 @@
    +   * @var \Drupal\migrate\Plugin\migrate\process\Iterator
    

    Desc missing.

  9. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/IteratorTest.php
    @@ -0,0 +1,88 @@
    +  protected $migrationConfiguration = array(
    

    @var doc block missing.

chx’s picture

FileSize
5 KB
2.23 KB
dawehner’s picture

Point 1.2 would be still nice.

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
469 bytes
5.04 KB
chx’s picture

Status: Reviewed & tested by the community » Needs review

Opsie.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +PHPUnit

Thank you

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, chx spent a great deal of time with me walking me through this so I could understand it. Well, sort of. :) Made some docs improvements along the way, along with a @see to https://drupal.org/node/2129651 where this is all explained in a lot more details.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.