Problem/Motivation

GitlabCI reports the following PHPCS issues:

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
Drupal.Files.LineLength.TooLong                                  14
Drupal.Arrays.Array.LongLineDeclaration                          6
Drupal.Commenting.FunctionComment.MissingReturnComment           5
Drupal.Commenting.DocComment.Empty                               3
Drupal.Semantics.FunctionT.NotLiteralString                      3
Drupal.Commenting.FunctionComment.MissingParamType               1
Drupal.NamingConventions.ValidFunctionName.InvalidPrefix         1
Generic.CodeAnalysis.UselessOverridingMethod.Found               1
Squiz.Arrays.ArrayDeclaration.NoKeySpecified                     1
----------------------------------------------------------------------
A TOTAL OF 35 SNIFF VIOLATIONS WERE FOUND IN 9 SOURCES
----------------------------------------------------------------------

The top two both relate to line lengths. This issue aims to resolve those.

Steps to reproduce

Run PHPCS. See errors reported.

Proposed resolution

For this issue, fix Files.LineLength.TooLong and Array.LongLineDeclaration. Other PHPCS issues will be fixed in follow on issues.

Issue fork project_browser-3419925

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

lostcarpark created an issue. See original summary.

lostcarpark opened merge request !429

lostcarpark’s picture

lostcarpark commented

This one is all long line issues. The bulk of them are comments over 80 chars.

Where it makes sense, I've tried to shorten the comment by removing unnecessary words.

Others I've reformatted and split over multiple lines.

There a few that I hope will be okay, such as splitting cspell:ignore, which I think are okay to split into multiple directives.

Also includes long array declarations.

lostcarpark’s picture

lostcarpark commented
Status: Active » Needs review
quietone’s picture

quietone commented
Status: Needs review » Needs work

@lostcarpark, thanks for this!

Setting to NW for the comments in the MR.

lostcarpark’s picture

lostcarpark commented
Status: Needs work » Needs review

Setting back to Needs Review.

There is one point @quietone raised that I haven't implemented: Drush command examples.

Actually, maybe this should just ignore the sniff that is failing? Something like phpcs:ignore Drupal.Commenting.FunctionComment.VoidReturn

Would this disable checks for that error? I'm concerned that would cause it to be ignored in places it should be picked up.

fjgarlin’s picture

fjgarlin commented
Status: Needs review » Needs work

Isn't that Commenting.FunctionComment.VoidReturn error out of the scope for this issue? As I read it it's only focusing in line-length-related errors.

Also, there are 14 threads open in the MR, so it's difficult to review it all without knowing all the context. It'd be great if those threads can be closed (or marked with a ✅ if you don't have permissions to close them) to know that the issue there was addressed.

Marking as NW based on the above and once those two things are clarified we can review further.

lostcarpark’s picture

lostcarpark commented

I have resolved all except 2 of the threads, mostly where I have applied the suggested fix.

The two remaining threads, I think I've done what was suggested, but I feel someone else should review.

lostcarpark’s picture

lostcarpark commented
Status: Needs work » Needs review
fjgarlin’s picture

fjgarlin commented

There are two pending threads, which are addressed, so they just need to either give more feedback or be closed.
From my side, this is RTBC, but I'll leave @quietone do it based on those two threads.

chrisfromredfin made their first commit to this issue’s fork.

quietone’s picture

quietone commented
Status: Needs review » Needs work

Sorry, one more!

lostcarpark’s picture

lostcarpark commented
Status: Needs work » Needs review

Accepting @Quietone's recommendation (with Chris's alteration).

Setting back to "Needs review".

lostcarpark’s picture

lostcarpark commented

I have rebased.

Brought in a couple of CSS changes I wasn't expecting. I think they were from other commits.

quietone’s picture

quietone commented
Status: Needs review » Needs work

Just a few more tweaks to go. I think some out of scope changes crept in as well.

lostcarpark’s picture

lostcarpark commented

I think the issues should all be fixed now.

quietone’s picture

quietone commented
Status: Needs work » Reviewed & tested by the community

I reviewed the last three commits in the MR, which are those added since my last review. All my points have been fixed and this is good to go. Nice to see this tidy up.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Reviewed & tested by the community » Fixed

Wonderful, onward to the remaining ones!

Status: Fixed » Closed (fixed)

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