Allow graphql pretty formatted files #466

p-makowski · 2023-09-13T20:39:42Z

Currently checks for GraphQL ValidTypeName assumes that type definitions are always one-liners.
For a better readability it is much better to format GraphQL files so that they are multiline.

type CartCampaign
    @doc(
        description: "CartCampaign returns the discount amount set in the quote"
    ) {
    full_discount_amount: Float
}

instead of

type CartCampaign @doc(description: "CartCampaign returns the discount amount set in the quote") {
    full_discount_amount: Float
}

There can many other tags like @doc and in current implementation all of them have to be in one line together with type to satisfy sniff. Changes in this PR will make pretty version to pass checks.

\PHP_CodeSniffer\Tokenizers\GRAPHQL::tokenize adds \n to token content anyway (see vendor/magento/magento-coding-standard/PHP_CodeSniffer/Tokenizers/GRAPHQL.php:134) to not screw up with line numbers.

This PR Closes #465

…e type definitions

…e type definitions - unit tests adjusted

fredden · 2023-09-14T23:49:57Z

Magento2/Sniffs/GraphQL/ValidTypeNameSniff.php

@@ -32,7 +32,7 @@ public function process(File $phpcsFile, $stackPtr)
        //compose entity name by making use of the next strings that we find until we hit a non-string token
        $name = '';
        for ($i=$stackPtr+1; $tokens[$i]['code'] === T_STRING; ++$i) {
-            $name .= $tokens[$i]['content'];
+            $name .= rtrim($tokens[$i]['content']);


Do we want to also allow white-space on the left of the name?

Suggested change

$name .= rtrim($tokens[$i]['content']);

$name .= trim($tokens[$i]['content']);

I think that tokenizer is never gonna create a T_STRING token with a whitespace on the left.

We should either fix the tokeniser to never include (leading nor) trailing whitespace in a T_STRING token, or run this through trim(). Using rtrim() seems incomplete.

Thanks for the explanation and highlighting where the extra white-space comes from (ie, the GraphQL tokeniser within this repository). Let's add a code comment here so others know why removing the trailing newline character(s) is safe in this context.

fredden · 2023-09-14T23:52:00Z

@p-makowski please can you use a closing keyword for #465 here.

p-makowski · 2023-09-15T06:59:44Z

@fredden I added issue id in my commits, so I believe issue and PR are linked already.
But sure, I can add it in case it is not enough.

p-makowski · 2023-09-15T07:00:00Z

Closes #465

fredden · 2023-09-19T14:03:34Z

I added issue id in my commits, so I believe issue and PR are linked already. But sure, I can add it in case it is not enough.
Closes #465

Please can you put this comment into the pull request description. In order for GitHub to work its magic, the closing keyword needs to be in the commit message (not just a reference to the issue, but a closing keyword), or the pull request description (not just a comment).

p-makowski · 2023-09-19T14:13:29Z

@fredden It was unclear to me when I read the documentation if it is about description, comments, commits...
Thank you for clarification. I edited PR description, but I am still unsure if it is linked.

Edit: OK, I see now that it is linked (there is a label when I hover over the line I added to the descrpition)

fredden · 2023-09-19T14:47:33Z

Great, thanks for your help on this @p-makowski. I can see that GitHub has linked these up, so when this pull request gets merged in, the issue will be automatically closed.

fredden

I wonder if this is instead a bug in the tokeniser.

fredden · 2023-10-25T14:13:44Z

Magento2/Sniffs/GraphQL/ValidTypeNameSniff.php

@@ -32,7 +32,7 @@ public function process(File $phpcsFile, $stackPtr)
        //compose entity name by making use of the next strings that we find until we hit a non-string token
        $name = '';
        for ($i=$stackPtr+1; $tokens[$i]['code'] === T_STRING; ++$i) {
-            $name .= $tokens[$i]['content'];
+            $name .= rtrim($tokens[$i]['content']);


We should either fix the tokeniser to never include (leading nor) trailing whitespace in a T_STRING token, or run this through trim(). Using rtrim() seems incomplete.

p-makowski · 2023-10-25T14:26:53Z

I wonder if this is instead a bug in the tokeniser.

@fredden see the last sentence in PR description:

\PHP_CodeSniffer\Tokenizers\GRAPHQL::tokenize adds \n to token content anyway (see vendor/magento/magento-coding-standard/PHP_CodeSniffer/Tokenizers/GRAPHQL.php:134) to not screw up with line numbers.

EOL char is added there on purpose "otherwise PHP_CodeSniffer will screw up line numbers".
So it looks like it was educated decision.

Regarding trim vs. rtrim - I am not sure if I want to open additional space for regression bugs.
I just do not have enough experience with coding standards to figure out all scenarios of what I can possibly break by trimming also on the beginning of the string. My gut feeling is that it should be OK, but on the other hand it does not relate anyhow to the specific issue that this PR solves. So the question is if it is worth the risk?

fredden

Thanks for the quick reply. I did miss that line in the pull request description, so thanks for highlighting it. I've reviewed the tokeniser code now, and using rtrim() makes sense to me now.

fredden · 2023-10-25T14:32:03Z

Magento2/Sniffs/GraphQL/ValidTypeNameSniff.php

@@ -32,7 +32,7 @@ public function process(File $phpcsFile, $stackPtr)
        //compose entity name by making use of the next strings that we find until we hit a non-string token
        $name = '';
        for ($i=$stackPtr+1; $tokens[$i]['code'] === T_STRING; ++$i) {
-            $name .= $tokens[$i]['content'];
+            $name .= rtrim($tokens[$i]['content']);


Thanks for the explanation and highlighting where the extra white-space comes from (ie, the GraphQL tokeniser within this repository). Let's add a code comment here so others know why removing the trailing newline character(s) is safe in this context.

p-makowski · 2023-10-30T17:23:31Z

Code comment has been added @fredden

p-makowski · 2023-12-01T19:58:55Z

Hi @fredden , any ETA on getting it out?

fredden · 2023-12-06T20:36:28Z

@p-makowski I have no control over Adobe's release process.

p-makowski · 2023-12-06T20:45:34Z

@p-makowski I have no control over Adobe's release process.

Oh, sorry @fredden! Somehow I was sure that you are part of maintaining team

p-makowski · 2024-01-04T09:47:22Z

@sidolov When can we get any attention and hopefully a release?

p-makowski added 2 commits September 13, 2023 22:26

[BUGFIX] magento#465 Trim tokens from graphql files to allow multilin…

45023fd

…e type definitions

[BUGFIX] magento#465 Trim tokens from graphql files to allow multilin…

f95a70e

…e type definitions - unit tests adjusted

m2-github-services added partners-contribution Partner: Visma Digital Commerce labels Sep 13, 2023

m2-community-project bot added this to Ready for Review in Pull Request Progress Sep 13, 2023

m2-community-project bot added the Progress: ready for review label Sep 13, 2023

fredden reviewed Sep 14, 2023

View reviewed changes

m2-community-project bot moved this from Ready for Review to Review in Progress in Pull Request Progress Sep 14, 2023

m2-community-project bot added Progress: review and removed Progress: ready for review labels Sep 14, 2023

m2-community-project bot moved this from Review in Progress to Ready for Review in Pull Request Progress Sep 15, 2023

m2-community-project bot added Progress: ready for review and removed Progress: review labels Sep 15, 2023

zukovasmartynas approved these changes Oct 24, 2023

View reviewed changes

m2-community-project bot moved this from Ready for Review to Reviewer Approved in Pull Request Progress Oct 24, 2023

m2-community-project bot added Progress: approved and removed Progress: ready for review labels Oct 24, 2023

jubi91 approved these changes Oct 24, 2023

View reviewed changes

p-makowski requested a review from fredden October 25, 2023 14:11

fredden reviewed Oct 25, 2023

View reviewed changes

m2-community-project bot assigned fredden Oct 25, 2023

m2-community-project bot moved this from Reviewer Approved to Review in Progress in Pull Request Progress Oct 25, 2023

m2-community-project bot added Progress: review and removed Progress: approved labels Oct 25, 2023

fredden reviewed Oct 25, 2023

View reviewed changes

[CHORE] magento#465 Added comment about code change

93b8e20

fredden approved these changes Oct 30, 2023

View reviewed changes

m2-community-project bot moved this from Review in Progress to Reviewer Approved in Pull Request Progress Oct 30, 2023

m2-community-project bot added Progress: approved and removed Progress: review labels Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow graphql pretty formatted files #466

Allow graphql pretty formatted files #466

p-makowski commented Sep 13, 2023 •

edited

fredden Sep 14, 2023

p-makowski Sep 15, 2023

fredden Oct 25, 2023

fredden Oct 25, 2023

fredden commented Sep 14, 2023

p-makowski commented Sep 15, 2023

p-makowski commented Sep 15, 2023

fredden commented Sep 19, 2023

p-makowski commented Sep 19, 2023 •

edited

fredden commented Sep 19, 2023

fredden left a comment

fredden Oct 25, 2023

p-makowski commented Oct 25, 2023 •

edited

fredden left a comment

fredden Oct 25, 2023

p-makowski commented Oct 30, 2023

p-makowski commented Dec 1, 2023 •

edited

fredden commented Dec 6, 2023

p-makowski commented Dec 6, 2023

p-makowski commented Jan 4, 2024

	$name .= rtrim($tokens[$i]['content']);
	$name .= trim($tokens[$i]['content']);

Allow graphql pretty formatted files #466

Are you sure you want to change the base?

Allow graphql pretty formatted files #466

Conversation

p-makowski commented Sep 13, 2023 • edited

fredden Sep 14, 2023

Choose a reason for hiding this comment

p-makowski Sep 15, 2023

Choose a reason for hiding this comment

fredden Oct 25, 2023

Choose a reason for hiding this comment

fredden Oct 25, 2023

Choose a reason for hiding this comment

fredden commented Sep 14, 2023

p-makowski commented Sep 15, 2023

p-makowski commented Sep 15, 2023

fredden commented Sep 19, 2023

p-makowski commented Sep 19, 2023 • edited

fredden commented Sep 19, 2023

fredden left a comment

Choose a reason for hiding this comment

fredden Oct 25, 2023

Choose a reason for hiding this comment

p-makowski commented Oct 25, 2023 • edited

fredden left a comment

Choose a reason for hiding this comment

fredden Oct 25, 2023

Choose a reason for hiding this comment

p-makowski commented Oct 30, 2023

p-makowski commented Dec 1, 2023 • edited

fredden commented Dec 6, 2023

p-makowski commented Dec 6, 2023

p-makowski commented Jan 4, 2024

p-makowski commented Sep 13, 2023 •

edited

p-makowski commented Sep 19, 2023 •

edited

p-makowski commented Oct 25, 2023 •

edited

p-makowski commented Dec 1, 2023 •

edited