magento-coding-standard
magento-coding-standard copied to clipboard
Allow graphql pretty formatted files
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
@p-makowski please can you use a closing keyword for #465 here.
@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.
Closes #465
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).
@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)
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.
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?
Code comment has been added @fredden
Hi @fredden , any ETA on getting it out?
@p-makowski I have no control over Adobe's release process.
@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
@sidolov When can we get any attention and hopefully a release?