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

Implementing Feature - getGraph() #87

Merged
merged 30 commits into from Aug 10, 2016

Conversation

Projects
None yet
2 participants
@rakesh-verma-16
Contributor

rakesh-verma-16 commented Jul 24, 2016

No description provided.

Show outdated Hide outdated src/Entity/Index/RevisionTreeIndex.php
*/
protected function generateEdges($revisions_array, $tree, $parent = -1 ) {
foreach ($tree as $item) {
$current_id =$item['#rev'];

This comment has been minimized.

@jeqq

jeqq Jul 26, 2016

Collaborator

Here is a missing space after =.

@jeqq

jeqq Jul 26, 2016

Collaborator

Here is a missing space after =.

Show outdated Hide outdated src/Entity/Index/RevisionTreeIndex.php
$rev_ids = array();
$this->storeNodesId($tree, $rev_ids);
$vertices = $this->generateVertices($graph, $rev_ids);
$this->generateEdges($vertices,$tree);

This comment has been minimized.

@jeqq

jeqq Jul 26, 2016

Collaborator

Missing space after ,

@jeqq

jeqq Jul 26, 2016

Collaborator

Missing space after ,

Show outdated Hide outdated src/Entity/Index/RevisionTreeIndex.php
@@ -65,6 +66,65 @@ public function getTree($uuid) {
return $values['tree'];
}
public function getGraph($uuid) {

This comment has been minimized.

@jeqq

jeqq Jul 26, 2016

Collaborator

Add getGraph() method to the RevisionTreeIndexInterface and add phpdoc comments for this method. Fix the format of the comments, they don't follow the Drupal coding standards.

@jeqq

jeqq Jul 26, 2016

Collaborator

Add getGraph() method to the RevisionTreeIndexInterface and add phpdoc comments for this method. Fix the format of the comments, they don't follow the Drupal coding standards.

Show outdated Hide outdated src/Tests/GraphCreationTest.php
*
* @group multiversion
*/
class GraphCreationTest extends MultiversionWebTestBase

This comment has been minimized.

@jeqq

jeqq Jul 26, 2016

Collaborator

Use Drupal coding standards to format the code: https://www.drupal.org/coding-standards

@jeqq

jeqq Jul 26, 2016

Collaborator

Use Drupal coding standards to format the code: https://www.drupal.org/coding-standards

Show outdated Hide outdated src/Tests/GraphCreationTest.php
$this->assertEqual($parent->getId(), $revs[8], 'node 10\'s parent is 8');
}
}
}

This comment has been minimized.

@jeqq

jeqq Jul 26, 2016

Collaborator

Missing new line.

@jeqq

jeqq Jul 26, 2016

Collaborator

Missing new line.

Show outdated Hide outdated src/Tests/GraphCreationTest.php
$this->assertEqual($parent->getId(), $revs[2],'node 4\'s parent is 3');
}
foreach ($vertices[$revs[4]]->getVerticesEdgeFrom() as $parent) {
$this->assertEqual($parent->getId(), $revs[3], 'node 5\'s parent is 2');

This comment has been minimized.

@jeqq

jeqq Jul 26, 2016

Collaborator

Here should be expected $revs[1], not $revs[3]

@jeqq

jeqq Jul 26, 2016

Collaborator

Here should be expected $revs[1], not $revs[3]

Show outdated Hide outdated src/Tests/GraphCreationTest.php
$entity->save();
$revs[] = $entity->_rev->value;
$entity = $storage->load(1);

This comment has been minimized.

@jeqq

jeqq Jul 26, 2016

Collaborator

Here should be loadRevision(1)

@jeqq

jeqq Jul 26, 2016

Collaborator

Here should be loadRevision(1)

Show outdated Hide outdated multiversion.info.yml
@@ -7,4 +7,4 @@ dependencies:
- entity_storage_migrate
- key_value
- serialization
- system (>=8.1.0)
- system (>=8.1.0)

This comment has been minimized.

@jeqq

jeqq Aug 1, 2016

Collaborator

Add the empty line back.

@jeqq

jeqq Aug 1, 2016

Collaborator

Add the empty line back.

Show outdated Hide outdated src/Entity/Index/ComplexLcaResolver.php
$vertices = $graph->getVertices()->getMap();
return $lca->find($vertices[$revision1->_rev->value], $vertices[$revision2->_rev->value]);
}
}

This comment has been minimized.

@jeqq

jeqq Aug 1, 2016

Collaborator

Missing empty line.

@jeqq

jeqq Aug 1, 2016

Collaborator

Missing empty line.

Show outdated Hide outdated src/Tests/ComplexLcaResolverTest.php
->loadRevision($parent_revision_id1);
$this->assertEqual($revisionLca['#rev'], $revs[2], "Yes we got it");
}
}

This comment has been minimized.

@jeqq

jeqq Aug 1, 2016

Collaborator

Missing empty line.

@jeqq

jeqq Aug 1, 2016

Collaborator

Missing empty line.

Show outdated Hide outdated src/Tests/ComplexLcaResolverTest.php
class ComplexLcaResolverTest extends MultiversionWebTestBase
{
public static $modules = ['entity_test', 'key_value', 'entity_storage_migrate', 'multiversion', 'conflict'];

This comment has been minimized.

@jeqq

jeqq Aug 1, 2016

Collaborator

To run this test in Travis CI you should add Conflict module as dependence in drupal-8.1.x.make.yml and drupal-8.2.x.make.yml.

@jeqq

jeqq Aug 1, 2016

Collaborator

To run this test in Travis CI you should add Conflict module as dependence in drupal-8.1.x.make.yml and drupal-8.2.x.make.yml.

Show outdated Hide outdated src/Tests/ComplexLcaResolverTest.php
/**
* {@inheritdoc}
*/
protected function setUp()

This comment has been minimized.

@jeqq

jeqq Aug 1, 2016

Collaborator

Codding standards? Please check all files you add or modify (better do this before committing).

@jeqq

jeqq Aug 1, 2016

Collaborator

Codding standards? Please check all files you add or modify (better do this before committing).

Show outdated Hide outdated src/Entity/Index/ComplexLcaResolver.php
return TRUE;
}
public function resolve(RevisionableInterface $revision1, RevisionableInterface $revision2, Graph $graph) {

This comment has been minimized.

@jeqq

jeqq Aug 2, 2016

Collaborator

Declaration is not compatible because in ConflictAncestorResolverInterface::resolve() you have as parameter Graph $graph = NULL. Here you have just Graph $graph.

@jeqq

jeqq Aug 2, 2016

Collaborator

Declaration is not compatible because in ConflictAncestorResolverInterface::resolve() you have as parameter Graph $graph = NULL. Here you have just Graph $graph.

Show outdated Hide outdated .travis.yml
@@ -31,6 +31,7 @@ before_install:
- wget -P ~/ https://github.com/drush-ops/drush/releases/download/8.0.0-rc3/drush.phar && chmod +x ~/drush.phar
- php ~/drush.phar make $TRAVIS_BUILD_DIR/.travis/$MAKE_FILE $HOME/www
- php ~/drush.phar --root=$HOME/www --yes site-install --db-url=mysql://root:@127.0.0.1/drupal testing
- composer --working-dir=$HOME/www require graphp/algorithms

This comment has been minimized.

@jeqq

jeqq Aug 3, 2016

Collaborator

Requre relaxedws/lca instead graphp/algorithms.

@jeqq

jeqq Aug 3, 2016

Collaborator

Requre relaxedws/lca instead graphp/algorithms.

Show outdated Hide outdated composer.json
@@ -2,5 +2,14 @@
"name": "drupal/multiversion",
"description": "Extends the revision support for content entities.",
"type": "drupal-module",
"repositories": [

This comment has been minimized.

@jeqq

jeqq Aug 3, 2016

Collaborator

Remove repositories, relaxedws/lca has been already added to packagist.

@jeqq

jeqq Aug 3, 2016

Collaborator

Remove repositories, relaxedws/lca has been already added to packagist.

Show outdated Hide outdated src/Tests/ComplexLcaResolverTest.php
@@ -91,7 +90,7 @@ public function testLcaFinder() {
$this->assertEqual($entity->getRevisionId(), 5, 'Default revision has been set correctly.');
// Create a new branch based on the first revision.
$entity = $storage->loadRevision(1);
$entity = $storage->load(1);

This comment has been minimized.

@jeqq

jeqq Aug 3, 2016

Collaborator

The comment doesn't correspond to the code you added.

@jeqq

jeqq Aug 3, 2016

Collaborator

The comment doesn't correspond to the code you added.

This comment has been minimized.

@rakesh-verma-16

rakesh-verma-16 Aug 3, 2016

Contributor

Thanks, resolved it.

@rakesh-verma-16

rakesh-verma-16 Aug 3, 2016

Contributor

Thanks, resolved it.

Show outdated Hide outdated src/Entity/Index/ComplexLcaResolver.php
public function resolve(RevisionableInterface $revision1, RevisionableInterface $revision2, Graph $graph = NULL) {
$lca = new LowestCommonAncestor($graph);
$vertices = $graph->getVertices()->getMap();

This comment has been minimized.

@rakesh-verma-16

rakesh-verma-16 Aug 3, 2016

Contributor

Indentation to be corrected.

@rakesh-verma-16

rakesh-verma-16 Aug 3, 2016

Contributor

Indentation to be corrected.

Show outdated Hide outdated src/Tests/ComplexLcaResolverTest.php
$parent_revision_id1 = $manager->resolveLowestCommonAncestor($revision1,$revision2, $graph);
// $revisionLca = Drupal::entityTypeManager()
// ->getStorage('entity_test')
// ->loadRevision($parent_revision_id1);

This comment has been minimized.

@rakesh-verma-16

rakesh-verma-16 Aug 3, 2016

Contributor

Comments to be removed.

@rakesh-verma-16

rakesh-verma-16 Aug 3, 2016

Contributor

Comments to be removed.

Show outdated Hide outdated src/Tests/ComplexLcaResolverTest.php
$manager = Drupal::service('conflict.lca_manager');
$parent_revision_id1 = $manager->resolveLowestCommonAncestor($revision1,$revision2, $graph);
// $revisionLca = Drupal::entityTypeManager()

This comment has been minimized.

@jeqq

jeqq Aug 10, 2016

Collaborator

Remove commented code.

@jeqq

jeqq Aug 10, 2016

Collaborator

Remove commented code.

Show outdated Hide outdated src/Tests/ComplexLcaResolverTest.php
$revision10 = Drupal::entityTypeManager()
->getStorage('entity_test')
->loadRevision(10);

This comment has been minimized.

@jeqq

jeqq Aug 10, 2016

Collaborator

Instead of lines 179 - 217 you can do:

$revisions = [];
    for ($i = 1; $i <= 10; $i++) {
      $revisions[$i] = $storage->load($i);
    }

And then you can use $revisions[1] instead of $revision1.

@jeqq

jeqq Aug 10, 2016

Collaborator

Instead of lines 179 - 217 you can do:

$revisions = [];
    for ($i = 1; $i <= 10; $i++) {
      $revisions[$i] = $storage->load($i);
    }

And then you can use $revisions[1] instead of $revision1.

Show outdated Hide outdated src/Tests/ComplexLcaResolverTest.php
$graph = $this->tree->getGraph($uuid);
$revision1 = Drupal::entityTypeManager()

This comment has been minimized.

@jeqq

jeqq Aug 10, 2016

Collaborator

The same here, you can do:

$revisions = [];
    for ($i = 1; $i <= 21; $i++) {
      $revisions[$i] = $storage->load($i);
    }
@jeqq

jeqq Aug 10, 2016

Collaborator

The same here, you can do:

$revisions = [];
    for ($i = 1; $i <= 21; $i++) {
      $revisions[$i] = $storage->load($i);
    }
Show outdated Hide outdated src/Tests/ComplexLcaResolverTest.php
$graph = $this->tree->getGraph($uuid);
$manager = Drupal::service('conflict.lca_manager');

This comment has been minimized.

@jeqq

jeqq Aug 10, 2016

Collaborator

Add $this->conflictLcaManager = $this->container->get('conflict.lca_manager'); in setUp() and use it instead of declaring it here (like this: $this->conflictLcaManager->resolveLowestCommonAncestor($revision1,$revision2, $graph);). Replace it everywhere in your code.

@jeqq

jeqq Aug 10, 2016

Collaborator

Add $this->conflictLcaManager = $this->container->get('conflict.lca_manager'); in setUp() and use it instead of declaring it here (like this: $this->conflictLcaManager->resolveLowestCommonAncestor($revision1,$revision2, $graph);). Replace it everywhere in your code.

Show outdated Hide outdated src/Tests/ComplexLcaResolverTest.php
$this->assertEqual($lca_id->getId(), $revs[2]);
}
// Graph structure in /vendor/relaxedws/lca/pictures/simple_graph.png

This comment has been minimized.

@jeqq

jeqq Aug 10, 2016

Collaborator

Indentation issue.

@jeqq

jeqq Aug 10, 2016

Collaborator

Indentation issue.

Show outdated Hide outdated src/Tests/ComplexLcaResolverTest.php
// Loading and storing revisions in $revision array.
$revision = [];

This comment has been minimized.

@jeqq

jeqq Aug 10, 2016

Collaborator

No need for this empty line.

@jeqq

jeqq Aug 10, 2016

Collaborator

No need for this empty line.

Show outdated Hide outdated src/Tests/GraphCreationTest.php
@@ -0,0 +1,353 @@
<?php
/**

This comment has been minimized.

@jeqq

jeqq Aug 10, 2016

Collaborator

Remove @file block, in Drupal core and contrib modules it should be removed.

@jeqq

jeqq Aug 10, 2016

Collaborator

Remove @file block, in Drupal core and contrib modules it should be removed.

Show outdated Hide outdated src/Tests/ComplexLcaResolverTest.php
@@ -0,0 +1,340 @@
<?php
/**

This comment has been minimized.

@jeqq

jeqq Aug 10, 2016

Collaborator

remove @file block.

@jeqq

jeqq Aug 10, 2016

Collaborator

remove @file block.

Show outdated Hide outdated src/Entity/Index/ComplexLcaResolver.php
use Relaxed\LCA\LowestCommonAncestor;
use Fhaculty\Graph\Graph;
use Relaxed\LCA\LcaException;
use Drupal\Multiversion\Entity\Index\RevisionTreeIndex;
class ComplexLcaResolver implements ConflictAncestorResolverInterface {
/**
* @return bool
*/
public function applies() {

This comment has been minimized.

@jeqq

jeqq Aug 10, 2016

Collaborator

Is should have just {@inheritdoc} because ConflictAncestorResolverInterface::apply()already has a phpdoc: https://github.com/drupaldeploy/drupal-conflict/blob/8.x-1.x/src/ConflictAncestorResolverInterface.php#L15-L18

@jeqq

jeqq Aug 10, 2016

Collaborator

Is should have just {@inheritdoc} because ConflictAncestorResolverInterface::apply()already has a phpdoc: https://github.com/drupaldeploy/drupal-conflict/blob/8.x-1.x/src/ConflictAncestorResolverInterface.php#L15-L18

Show outdated Hide outdated src/Entity/Index/ComplexLcaResolver.php
* @param RevisionableInterface $revision2
* @param Graph $graph
*
* @return Vertex

This comment has been minimized.

@jeqq

jeqq Aug 10, 2016

Collaborator

It returns an array of vertices or an empty array, not an object of Vertex type.

@jeqq

jeqq Aug 10, 2016

Collaborator

It returns an array of vertices or an empty array, not an object of Vertex type.

Show outdated Hide outdated src/Tests/ComplexLcaResolverTest.php
}
// Graph structure in multiversion/vendor/relaxedws/lca/pictures/simple_graph.png
public function testLcaFinder3() {

This comment has been minimized.

@jeqq

jeqq Aug 10, 2016

Collaborator

Indentation issue.

@jeqq

jeqq Aug 10, 2016

Collaborator

Indentation issue.

$this->assertEqual($parent->getId(), $revs[7], 'node 10\'s parent is 8');
}
}

This comment has been minimized.

@jeqq

jeqq Aug 10, 2016

Collaborator

Remove one empty line.

@jeqq

jeqq Aug 10, 2016

Collaborator

Remove one empty line.

Show outdated Hide outdated src/Tests/ComplexLcaResolverTest.php
@@ -206,7 +201,7 @@ public function testLcaFinder2() {
$this->assertEqual($lca_id->getId(), $revs[2]);
}
// Graph structure in multiversion/vendor/relaxedws/lca/pictures/simple_graph.png
// Graph structure in multiversion/vendor/relaxedws/lca/pictures/simple_graph.png

This comment has been minimized.

@jeqq

jeqq Aug 10, 2016

Collaborator

Now you have 2 lines with wrong indentation. Please fix it.

@jeqq

jeqq Aug 10, 2016

Collaborator

Now you have 2 lines with wrong indentation. Please fix it.

@jeqq jeqq merged commit faaa051 into dickolsson:8.x-1.x Aug 10, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment