In devel/commit/242229e phpcs checking of .info .txt .md and .yml files was removed. The reason was "PHPCS should ignore .md files. Avoids fail reports in MRs"

However, there are probably some valid reason for checking all these file types. Many of the coding standards faults could/can be fixed. By not checking any of these file types we risk missing something important.

So there is benefit here in checking exactly what the failures are. It may be that we decide to skip an individual file, or a particular rule, but we need to see exactly what the problem is first.

CommentFileSizeAuthor
#8 3133703-8.run-phpcs-on-txt-info-md-yml.patch3.57 KBjonathan1055
#7 3133703-7.run-phpcs-on-txt-info-md-yml.patch4.11 KBjonathan1055
#6 3133703-6.run-phpcs-on-md-yml.patch3.62 KBjonathan1055
#3 3133703-3.run-phpcs-on-txt-info-md-yml.patch1.47 KBjonathan1055
#2 3133703-2.run-phpcs-on-txt-info-md-yml.patch1.05 KBjonathan1055

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review
StatusFileSize
new 3133703-2.run-phpcs-on-txt-info-md-yml.patch1.05 KB

Here's a patch which should check all of the files of these four types, but only these. Also it skips the full phpunit testing as that is unnecessary until the final check, and just wastes d.o. resources.

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 3133703-3.run-phpcs-on-txt-info-md-yml.patch1.47 KB

So we get

CONTRIBUTING.md ✗ 5 more
4	Line exceeds 80 characters; contains 88 characters
8	Line exceeds 80 characters; contains 100 characters
10	Line exceeds 80 characters; contains 83 characters
18	Line exceeds 80 characters; contains 129 characters
20	Line exceeds 80 characters; contains 87 characters
devel_generate/README.md ✗ 1 more 
16	Line exceeds 80 characters; contains 84 characters
README.md ✗ 1 more
22	Line exceeds 80 characters; contains 82 characters
webprofiler/README.md ✗ 9 more
3	Line exceeds 80 characters; contains 106 characters
8	Line exceeds 80 characters; contains 242 characters
13	Line exceeds 80 characters; contains 93 characters
46	Line exceeds 80 characters; contains 112 characters
66	Line exceeds 80 characters; contains 115 characters
75	Line exceeds 80 characters; contains 101 characters
77	Line exceeds 80 characters; contains 89 characters
80	Line exceeds 80 characters; contains 113 characters
84	Line exceeds 80 characters; contains 147 characters

Lines that end with a url are exempt from the 80 char limit, others can be fixed and the flow of the rendered markdown will not be affected. Or maybe we want to ignore the 80 char limit just for .md files?

moshe weitzman’s picture

moshe weitzman commented

Hi. Sorry I should have checked with you before changing this. I kind of assumed that neither PHPCS nor Drupal coding standards had much applicability to non-PHP files.

I think that 80 char limit is super outdated. Linus said so in 2009! https://lkml.org/lkml/2009/12/17/229

These violations are just frustrate devs. And they consume more CI minutes as devs retest to try to get to green. Since devel's phpcs.xml.dist is currently the model in the docs, I propose we either omit these file types, or exempt text files from line length rule. Either one is fine with me

jonathan1055’s picture

jonathan1055 commented

These violations just frustrate devs.

Yes I agree.

I propose we [... or] exempt text files from line length rule.

I will exempt text files from the 80 limit. Presume this is only for the 3.x branch? I have not been making any changes to 2.x apart from really obvious and necessary fixes. Actually, just realised we don't have a phpcs.xml.dist in 2.x as I only introduced that in Nov last year on 3.x So it's just 3.x then.

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 3133703-6.run-phpcs-on-md-yml.patch3.62 KB

Patch to fix the few things in .yml files

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 3133703-7.run-phpcs-on-txt-info-md-yml.patch4.11 KB

I noticed a few days ago that there was a drop in coding standards messages, but due to other files changing and moving I did not get to investigate it properly. I have now worked it through, and realised that a change from <exclude-pattern>*.patch</exclude-pattern> to <exclude-pattern>.patch</exclude-pattern> had the effect of excluding all files with 'patch' anywhere in the name. This meant that webprofiler/src/EventDispatcher/EventDispatcherTraceableInterface.php (5 faults) and webprofiler/src/EventDispatcher/TraceableEventDispatcher.php (30 faults) had got dropped from the report. Hence fixing that here by escaping the . with \.

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 3133703-8.run-phpcs-on-txt-info-md-yml.patch3.57 KB

We actually get the 36 extra messages for the two dispatcher files. TraceableEventDispatcher.php has 31 not 30 and the extra message is Line 68 Unused variable $key.. This is created by sniff DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable and I have seen this variation before, e.g patch #2 above, this sniff is not reported on, but in patch #7 it was.

Here's the patch to be committed, just to make sure the tests actually run OK.

  • jonathan1055 committed 9183972 on 8.x-3.x 
    Issue #3133703 by jonathan1055: Run PHPCS on .info .txt .md and .yml...
jonathan1055’s picture

jonathan1055 commented
Title: PHPCS on .info .txt .md and .yml files » Run PHPCS on .info .txt .md and .yml files but exclude 80 char limt on .md files
Status: Needs review » Fixed

I think there is no more to do here.

Issue also migrated to https://gitlab.com/drupalspoons/devel/-/issues/257 so I will close that too.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.