Clean coding standards are beneficial for readability, maintainability and code re-use, so we should try to remove these messages. The outcome should be that we do not see these any coding standards faults as a matter of course, as they can mask new faults that would be introduced without alert.

"As of Drupal 8, we use ESLint to make sure our JavaScript code is consistent and free from syntax error and leaking variables and that it can be properly minified" as stated on https://www.drupal.org/docs/develop/standards/javascript/eslint-settings

As at 3 Jan 2023 there are 770 coding standards faults on the 4.x branch (and same on 8.x-3.x). Following the successful php clean-up on #3106511: Drupal PHP coding standards the remaining errors are only within the javascript files:

#3338336: Javascript coding standards (uninstall, permissions, update status)
cover module_filter.modules_uninstall.js, module_filter.permissions.js and module_filter.update_status.js

#3338338: Javascript coding standards (winnow.js and module_filter.js)
cover jquery.winnow.js and module_filter.js

#3338339: Javascript coding standard (modules, modules_bare, modules_tab)
covers module_filter.modules.js, module_filter.modules_bare.js and module_filter.modules_tabs.js

Eslint also executes 'prettier' formatting, a subset of the coding standards. This runs on .js and .yml files
#3405485: Fix prettier formating warnings in javascript files
#3412888: Fix coding standards and prettier formatting in .yml files

Tasks

  1. Verify which coding standards are being used to generate these problems
  2. Be able to re-create the report locally to enable fixing and checking
  3. Decide whether we fix the files or simply remove the coding checks

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes

Updated IS with link to https://www.drupal.org/docs/develop/standards/javascript/eslint-settings

smustgrave’s picture

smustgrave commented

Won't lie there are so many errors I wonder if breaking each file out to it's own ticket would make sense.

jonathan1055’s picture

jonathan1055 commented

Yes we could split out into 8 issues, or do like core does and split by type of problem. Not sure which is best for javascript.

We could also hide the faults for 'prettier' formatting by adding *.js to the new .prettierignore file. See the result on #3333862-7: YML coding standards

As we only have a small amount of automated test coverage, we would not want to commit too many changes without comprehensive manual testing (or adding more automated tests).

smustgrave’s picture

smustgrave commented

Think adding test coverage while doing the standard would be a good idea.

Also good opportunity to remove code that's not needed.

Example the uninstall filter I think is on it's way out the door.

jonathan1055’s picture

jonathan1055 commented

"Example the uninstall filter I think is on it's way out the door."

I think we need to keep that because the core filter does not work properly. It ignores the first word of the description and does not match partial words unless you start at the begining of the word. For example if you enter query the core filter does not find jquery, but the module_filter version does. Until that is fixed in core we should keep this modules uninstall filter :-)

smustgrave’s picture

smustgrave commented

So are we in agreement to break up this ticket to cover each file (or 2 files a ticket) in separate issues.

jonathan1055’s picture

jonathan1055 commented

Yes definitely break it up into more manageble child issues. Maybe start with module_filter.modules_uninstall.js and module_filter.permissions.js because (a) they are smaller and focussed on a single task, and (b) we know we have test coverage for them.

Maybe one file per ticket is a good idea? I can see these issues taking a while to resolve, so one per ticket will minimise the rebasing we have to do if other commits are done in the meantime. We may also make commits part way through even if there are still some problems not resolved.

smustgrave’s picture

smustgrave commented
Title: Javascript coding standards » [META] Javascript coding standards
Issue summary: View changes

Opened up 3 sub tickets and changed this to a meta.

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes

Re-ordered to issue number, to match the sequence as shown in side block.

smustgrave’s picture

smustgrave commented

Wonder if we should restart some of these with gitlab? Plan on looking at this module some next week.

jonathan1055’s picture

jonathan1055 commented

Good idea. I have opened #3402091: Add gitlab_templates pipeline file

jonathan1055’s picture

jonathan1055 commented

The three coding standards child issues have now been completed.

But leaving this meta issue open as we still have #3405485: Fix prettier formating warnings in javascript files

smustgrave’s picture

smustgrave commented

We can use this meta for testing after the last one lands.

smustgrave’s picture

smustgrave commented
Status: Active » Needs review

Okay everything is merged and passing.

Lets use this ticket for some last minute testing of all the changes.

jonathan1055’s picture

jonathan1055 commented

I've added a new child issue #3412888: Fix coding standards and prettier formatting in .yml files because currently all .yml are ignored. We should be able to fix them, I've done it on other contrib modules.

jonathan1055’s picture

jonathan1055 commented
Title: [META] Javascript coding standards » [META] Javascript and YML coding standards
Issue summary: View changes

Updated IS.

smustgrave’s picture

smustgrave commented

So my initial testing I don't see anything broken. Think we are good to tag a release?

jonathan1055’s picture

jonathan1055 commented
Status: Needs review » Fixed

This meta is fixed. There are no others that are 'needs review' so nothing that is nearly ready. So yes a new release would be great, as 4.0.1 was released Dec 2022.

According to https://drupal-mrn.dev/ from 4.0.1 to 4.x there are 24 issues resolved. So were you thinking of releasing 4.0.2 or is there enough to warrant 4.1.0?

smustgrave’s picture

smustgrave commented
smustgrave’s picture

smustgrave commented

I'm thinking 4.1.0 as we changed a lot

smustgrave’s picture

smustgrave commented

Just tagged a release. Thanks for all the contribution!

jonathan1055’s picture

jonathan1055 commented

Great that we have a new release. It's a pleasure to help.

Status: Fixed » Closed (fixed)

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