Skip to content

InlineComment, VariableComment and NewlineAfterCloseBrace Rules removed#1062

Merged
rthideaway merged 2 commits intodrupal-graphql:8.x-4.xfrom
Dylan203:fix/cs-fixes-iteration-3
Aug 11, 2020
Merged

InlineComment, VariableComment and NewlineAfterCloseBrace Rules removed#1062
rthideaway merged 2 commits intodrupal-graphql:8.x-4.xfrom
Dylan203:fix/cs-fixes-iteration-3

Conversation

@Dylan203
Copy link
Copy Markdown
Contributor

exclude name="Drupal.Commenting.InlineComment.InvalidEndChar"
exclude name="Drupal.Commenting.InlineComment.NoSpaceBefore"
exclude name="Drupal.Commenting.InlineComment.SpacingAfter"
exclude name="Drupal.Commenting.InlineComment.SpacingBefore"
exclude name="Drupal.Commenting.InlineComment.SpacingBefore"
exclude name="Drupal.Commenting.VariableComment.IncorrectVarType"
exclude name="Drupal.Commenting.VariableComment.Missing"
exclude name="Drupal.ControlStructures.ControlSignature.NewlineAfterCloseBrace"

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 11, 2020

Codecov Report

Merging #1062 into 8.x-4.x will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             8.x-4.x    #1062   +/-   ##
==========================================
  Coverage      50.38%   50.38%           
  Complexity       683      683           
==========================================
  Files            118      118           
  Lines           1820     1820           
==========================================
  Hits             917      917           
  Misses           903      903           
Impacted Files Coverage Δ Complexity Δ
src/Entity/Server.php 87.27% <ø> (ø) 38.00 <0.00> (ø)
src/Form/PersistedQueriesForm.php 0.00% <ø> (ø) 18.00 <0.00> (ø)
.../Plugin/GraphQL/DataProducer/DataProducerProxy.php 40.20% <ø> (ø) 36.00 <0.00> (ø)
...L/DataProducer/Menu/MenuLink/MenuLinkAttribute.php 0.00% <ø> (ø) 2.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8d403b...ca698ca. Read the comment docs.

Copy link
Copy Markdown
Contributor

@rthideaway rthideaway left a comment

Choose a reason for hiding this comment

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

@Dylan203 looks already very good, thanks! found minor things, but also found a bigger issue with the test we need to fix, please follow the comments below

Comment thread tests/src/Kernel/EntityBufferTest.php Outdated
class EntityBufferTest extends GraphQLTestBase {

/**
* @var NodeIDsArray
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually I found out that the test is written incorrectly. Problem is with the following code in the file below:

    foreach (range(1, 3) as $i) {
      $this->nodeIds[] = Node::create([
        'title' => 'Node ' . $i,
        'type' => 'test',
      ])->save();
    }

Because save method below does not return entity (node) id, it returns the status of saving the entity (for new entities it returns 1, for updated entities returns 2, for deleted entities it returns 3). So the array is filled with three 1's which is something we don't want. Let's first fix the foreach loop bellow with the following form:

    foreach (range(1, 3) as $i) {
      $node = Node::create([
        'title' => 'Node ' . $i,
        'type' => 'test',
      ]);
      $node->save();
      $this->nodeIds[] = $node->id();
    }

And now let's fix this @var annotation. @var is describing the type of the variable, so it can be string, int, bool, or some specific object like \Drupal\Tests\graphql\Kernel\EntityBufferTest. In this case it's array. For array we always want to be as specific as possible. If it's array of booleans then we'll use bool[]. Here we know that $node->id() returns id as a string, so let's use this:

   * @var string[]

class PersistedQueriesTest extends GraphQLTestBase {

/**
* @var ModulesArray
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

here again, it's array of strings so lets use:

  * @var string[]

Copy link
Copy Markdown
Contributor

@rthideaway rthideaway left a comment

Choose a reason for hiding this comment

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

@Dylan203 thank you, now looks perfect! :)
now let's wait for the Travis builds and if they pass I will merge it :)

@rthideaway
Copy link
Copy Markdown
Contributor

@klausi I just noticed I cannot merge PRs in this module :( could you merge it instead?

@rthideaway rthideaway merged commit 5852b8a into drupal-graphql:8.x-4.x Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants