TypeHintMissing and ReturnTypeSpaces Rules removed#1063
TypeHintMissing and ReturnTypeSpaces Rules removed#1063rthideaway merged 3 commits intodrupal-graphql:8.x-4.xfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## 8.x-4.x #1063 +/- ##
==========================================
Coverage 50.38% 50.38%
Complexity 683 683
==========================================
Files 118 118
Lines 1820 1820
==========================================
Hits 917 917
Misses 903 903
Continue to review full report at Codecov.
|
rthideaway
left a comment
rthideaway
left a comment
There was a problem hiding this comment.
great! found just some small things :)
| * The http response object. | ||
| */ | ||
| protected function batchedQueries(array $queries, $server = NULL) { | ||
| protected function batchedQueries(array $queries, \Drupal\graphql\Entity\Server $server = NULL) { |
There was a problem hiding this comment.
There are two problems with $server argument here. First we shouldn't add a full namespace (fully qualified domain name) for method arguments. And second, we should use interface instead of specific class. Let's use interface instead, because if somebody wants to extend default Server class and use it, they wouldn't be able to do so. Let's use here ServerInterface, and make sure namespace is added to the top of the file:
use Drupal\graphql\Entity\ServerInterface;
| * The printed version of the schema. | ||
| */ | ||
| protected function getPrintedSchema($server = NULL) { | ||
| protected function getPrintedSchema(\Drupal\graphql\Entity\ServerInterface $server = NULL) { |
There was a problem hiding this comment.
similar here, let's use just ServerInterface and make sure namespace is added to the top of the file:
use Drupal\graphql\Entity\ServerInterface;
rthideaway
left a comment
rthideaway
left a comment
There was a problem hiding this comment.
@Dylan203 one more very small leftover
|
|
||
| use Drupal\graphql\Entity\Server; | ||
| use Symfony\Component\HttpFoundation\Request; | ||
| use Drupal\graphql\Entity\ServerInterface; |
There was a problem hiding this comment.
cool, what is usually done in the list of used namespaces is, that they are ordered alphabetically, so it's easier for a developer to look through the list. sometimes the list is really huge and if we are checking some namespaces it's harder to go through unordered list.
Hint: your IDE will most certainly help you with that. Almost every IDE is able to add a namespace with some shortcut to the correct position. If you are using PHPstorm you can try it yourself. Just delete the namespace here and go the method which is using it and click on it. Then you can use a shortcut Alt + Enter which pops up a dialog where you choose Import class:
You click on it and just select proper class from the list:
And boom the namespace is added to the top of the file to the correct position :)
|
|
||
| use Drupal\graphql\Entity\Server; | ||
| use GraphQL\Utils\SchemaPrinter; | ||
| use Drupal\graphql\Entity\ServerInterface; |
There was a problem hiding this comment.
same here, let's keep the list in alphabetical order
rthideaway
left a comment
rthideaway
left a comment
There was a problem hiding this comment.
looks good! thanks! :)
2 participants
exclude name="Drupal.Commenting.FunctionComment.ReturnTypeSpaces"
exclude name="Drupal.Commenting.FunctionComment.TypeHintMissing"