Skip to content

Conversation

jean-bernard-valentaten
Copy link
Contributor

Sniffing GraphQL schemas was added. The following rules/sniffs were added:

  • enum, type and interface names must be UpperCamelCase
  • Field names of type and interface must be snake_case
  • Argument names of fields must be camelCase
  • query and mutation names must be camelCase

In order to be able to tokenize a GraphQL file, we now make use of the lexer bundled in the package webonyx/graphql-php.

larsroettig
larsroettig previously approved these changes Sep 11, 2019
@lenaorobei
Copy link
Contributor

@jean-bernard-valentaten thanks, that is significant improvement!

Could you please demo it on public architecture meeting magento/architecture#273?

I see that some rules are still under discussion magento/architecture#269, so please be aware that we cannot merge it until final decision is made.

@lenaorobei lenaorobei added the on hold The issue or PR is on hold. label Sep 11, 2019
Copy link
Contributor Author

@jean-bernard-valentaten jean-bernard-valentaten left a comment

Choose a reason for hiding this comment

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

@lenaorobei Please discuss presentation of feature in architecture meeting with Igor Melnykov as he informed me, that it needs not be presented.

@lenaorobei
Copy link
Contributor

I pulled changes and run composer install. Running command php vendor/bin/phpcs --standard=Magento2 <path-to-my-magento>/app/code/Magento/CatalogGraphQl/etc/schema.graphqls gives me following error:
PHP Fatal error: Uncaught Error: Class 'PHP_CodeSniffer\Tokenizers\GRAPHQL' not found in magento-coding-standard/vendor/squizlabs/php_codesniffer/src/Files/File.php:556.

@jean-bernard-valentaten
Copy link
Contributor Author

@lenaorobei Thank you for the note, I fixed that issue and a couple of follow up issues

@lenaorobei
Copy link
Contributor

I run against Magento 2 app/code and following false-positive findings were discovered:

 FILE: app/code/Magento/CmsGraphQl/etc/schema.graphqls ---------------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ---------------------------------------------------------------------------------------------- 19 | WARNING | Argument name "String@doc(description" is not in CamelCase format ---------------------------------------------------------------------------------------------- FILE: app/code/Magento/CustomerGraphQl/etc/schema.graphqls ------------------------------------------------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 246 WARNINGS AFFECTING 246 LINES ------------------------------------------------------------------------------------------------------------------------------- 7 | WARNING | Argument name "@doc(description" is not in CamelCase format 138 | WARNING | Enum value "AF@doc(description:Afghanistan)" is not in SCREAMING_SNAKE_CASE format 139 | WARNING | Enum value "AX@doc(description:Åland Islands)" is not in SCREAMING_SNAKE_CASE format 140 | WARNING | Enum value "AL@doc(description:Albania)" is not in SCREAMING_SNAKE_CASE format ... ------------------------------------------------------------------------------------------------------------------------------- FILE: app/code/Magento/DownloadableGraphQl/etc/schema.graphqls ---------------------------------------------------------------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES ---------------------------------------------------------------------------------------------------------------------------------------------- 41 | WARNING | Enum value "FILE@deprecated(reason:`sample_url` serves to get the downloadable sample)" is not in SCREAMING_SNAKE_CASE format 42 | WARNING | Enum value "URL@deprecated(reason:`sample_url` serves to get the downloadable sample)" is not in SCREAMING_SNAKE_CASE format ---------------------------------------------------------------------------------------------------------------------------------------------- 

Please add these cases to the unit tests as well.
Thanks.

@jean-bernard-valentaten
Copy link
Contributor Author

jean-bernard-valentaten commented Sep 17, 2019

I run against Magento 2 app/code and following false-positive findings were discovered:

 FILE: app/code/Magento/CmsGraphQl/etc/schema.graphqls ---------------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ---------------------------------------------------------------------------------------------- 19 | WARNING | Argument name "String@doc(description" is not in CamelCase format ---------------------------------------------------------------------------------------------- FILE: app/code/Magento/CustomerGraphQl/etc/schema.graphqls ------------------------------------------------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 246 WARNINGS AFFECTING 246 LINES ------------------------------------------------------------------------------------------------------------------------------- 7 | WARNING | Argument name "@doc(description" is not in CamelCase format 138 | WARNING | Enum value "AF@doc(description:Afghanistan)" is not in SCREAMING_SNAKE_CASE format 139 | WARNING | Enum value "AX@doc(description:Åland Islands)" is not in SCREAMING_SNAKE_CASE format 140 | WARNING | Enum value "AL@doc(description:Albania)" is not in SCREAMING_SNAKE_CASE format ... ------------------------------------------------------------------------------------------------------------------------------- FILE: app/code/Magento/DownloadableGraphQl/etc/schema.graphqls ---------------------------------------------------------------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES ---------------------------------------------------------------------------------------------------------------------------------------------- 41 | WARNING | Enum value "FILE@deprecated(reason:`sample_url` serves to get the downloadable sample)" is not in SCREAMING_SNAKE_CASE format 42 | WARNING | Enum value "URL@deprecated(reason:`sample_url` serves to get the downloadable sample)" is not in SCREAMING_SNAKE_CASE format ---------------------------------------------------------------------------------------------------------------------------------------------- 

Please add these cases to the unit tests as well.
Thanks.

Fixed in af9c041 and c4069e0

I ran it (GraphQL analysis only!) against Magento 2 CE app/code and here is the output:

FILE: /home/valentatenj/workspace/adobe/magento2ce/app/code/Magento/VaultGraphQl/etc/schema.graphqls ---------------------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES ---------------------------------------------------------------------------------------------------- 5 | WARNING | Argument name "public_hash" is not in CamelCase format 29 | WARNING | Enum value "card" is not in SCREAMING_SNAKE_CASE format 30 | WARNING | Enum value "account" is not in SCREAMING_SNAKE_CASE format ---------------------------------------------------------------------------------------------------- FILE: /home/valentatenj/workspace/adobe/magento2ce/app/code/Magento/QuoteGraphQl/etc/schema.graphqls ---------------------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ---------------------------------------------------------------------------------------------------- 5 | WARNING | Argument name "cart_id" is not in CamelCase format ---------------------------------------------------------------------------------------------------- 
@lenaorobei
Copy link
Contributor

Please also address that following message appears on composer install step: Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. Run update to update them.

@jean-bernard-valentaten
Copy link
Contributor Author

Please also address that following message appears on composer install step: Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. Run update to update them.

Fixed in 4d99ee4

@lenaorobei lenaorobei merged commit 7813049 into develop Sep 25, 2019
@lenaorobei lenaorobei deleted the MC-19366-GraphQL-Code-Style-Test branch November 4, 2019 18:09
magento-devops-reposync-svc pushed a commit that referenced this pull request Dec 22, 2021
…kiy-magento-coding-standard-346 [Imported] Fix Issue 340
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

6 participants