Skip to content

Files excluded with Comment Rules#1073

Merged
klausi merged 7 commits intodrupal-graphql:8.x-4.xfrom
Dylan203:fix/cs-fixes-iteration-11
Aug 26, 2020
Merged

Files excluded with Comment Rules#1073
klausi merged 7 commits intodrupal-graphql:8.x-4.xfrom
Dylan203:fix/cs-fixes-iteration-11

Conversation

@Dylan203
Copy link
Copy Markdown
Contributor

exclude name="Drupal.Commenting.ClassComment.Missing"
exclude name="Drupal.Commenting.DocComment.MissingShort"
exclude name="Drupal.Commenting.FunctionComment.MissingParamComment"
exclude name="Drupal.Commenting.FunctionComment.MissingReturnComment"

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 18, 2020

Codecov Report

Merging #1073 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    #1073   +/-   ##
==========================================
  Coverage      50.51%   50.51%           
  Complexity       674      674           
==========================================
  Files            118      118           
  Lines           1827     1827           
==========================================
  Hits             923      923           
  Misses           904      904           

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 f928748...d842dac. 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.

well, okay, but I think, someone should fix those comments instead as this list makes the whole phpcs config look very ugly and too long. I'll try to do it as far as I find some time. for now it's ok

@rthideaway
Copy link
Copy Markdown
Contributor

@Dylan203 can you resolve the conflict please? then we can merge this as well

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.

looks good, small improvement

Comment thread phpcs.xml.dist Outdated
@@ -7,10 +7,305 @@

<rule ref="Drupal">
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.

as there is no rule anymore listeed here, let's fully remove this tag completely

Copy link
Copy Markdown
Contributor

@klausi klausi left a comment

Choose a reason for hiding this comment

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

@Dylan203 can you fix the merge conflicts and move the comment?

Comment thread phpcs.xml.dist Outdated
@@ -7,10 +7,305 @@

<rule ref="Drupal">
<!-- TODO: those rules are disabled for now until we fix the coding standards for them. -->
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.

this section is now empty, so the TODO comment should be moved/copied to the new exceptions we make below.

Copy link
Copy Markdown
Contributor

@klausi klausi left a comment

Choose a reason for hiding this comment

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

Thanks, some questions.

We should also reference the DurpalPractice standard and check if we have any violations there. We could do this also in this pull request if we don't have a lot of fixes for that.

Comment thread phpcs.xml.dist

<arg name="extensions" value="inc,install,module,php,profile,test,theme,yml"/>

<rule ref="Drupal">
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.

we need to keep this line, we want to reference the whole Drupal standard first.

Comment thread phpcs.xml.dist Outdated
<exclude-pattern>tests/src/Kernel/GraphQLTestBase.php</exclude-pattern>
</rule>

<exclude-pattern>graphql.info.yml</exclude-pattern>
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.

why do we exclude those yml files? Do we get any errors for them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If I remove phpcs is mad and gives out this error:

No PHP code was found in this file and short open tags are not allowed by this install of PHP. This file may be using short open tags but PHP does not allow them.

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.

Ah I see, because phpcs somehow throws errors for them. That happens because you did not refernece a standard, so once you bring back the Drupal reference we should not see this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Jup build works now

@klausi klausi merged commit 0ad5a8f into drupal-graphql:8.x-4.x Aug 26, 2020
@klausi
Copy link
Copy Markdown
Contributor

klausi commented Aug 26, 2020

Thank you!

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.

3 participants