Problem/Motivation

Fix the YML coding standards reported by ESLINT

Remaining tasks

  1. Fix eslint coding standards as reported on branch tests [done]
  2. Add .prettierrc.json to replicate core's config [done]
  3. Add .prettierignore to control which files are checked [done]
CommentFileSizeAuthor
#7 ignoring_prettier_for_js_files.jpeg479.41 KBjonathan1055

Issue fork module_filter-3333862

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

jonathan1055 created an issue. See original summary.

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

smustgrave opened merge request !18

smustgrave’s picture

smustgrave commented
Status: Active » Needs review

Added a .prettierrc.json file and only errors I'm getting are from drupalci.yml. prettierignore doesn't work though.

jonathan1055’s picture

jonathan1055 commented

OK I'll check what you have done and compare, as I had started this and had local changes ready.

jonathan1055’s picture

jonathan1055 commented

I'm not sure what you meant by "prettierignore doesn't work" but I have added my file. If none exists then the testbot creates one containing "*.yml". But we can probably fix the faults, so do not need to ignore all .yml

There is a bug in testbot script which means that only the changed js files are sent to eslint. Changed .yml files are ignored, but then you get a surprise when the branch test is run and they are included. I have reported it on #3333051: Modified .yml files are not passed to eslint and fixed the bug.

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new ignoring_prettier_for_js_files.jpeg479.41 KB

That's better. The test result is now running eslint on all files due to the prescence of .eslintrc.json in the MR changed files. We get 486 faults, down by 280.

smustgrave’s picture

smustgrave commented

I run npx eslint --ext yml web/modules/contrib/module_filter/ from my project root and get 


/Users/smustgrave/Sites/Drupal-Demos/drupal-10.x/web/modules/contrib/module_filter/drupalci.yml
   5:7  error  Empty mapping values are forbidden  yml/no-empty-mapping-value
   6:7  error  Empty mapping values are forbidden  yml/no-empty-mapping-value
   8:9  error  Empty mapping values are forbidden  yml/no-empty-mapping-value
  14:7  error  Empty mapping values are forbidden  yml/no-empty-mapping-value
  15:7  error  Empty mapping values are forbidden  yml/no-empty-mapping-value
  16:7  error  Empty mapping values are forbidden  yml/no-empty-mapping-value
  19:9  error  Empty mapping values are forbidden  yml/no-empty-mapping-value
jonathan1055’s picture

jonathan1055 commented

The steps need to appear in drupalci.yml, otherwise the test bot does not run them. We can resolve the empty mapping problem by specifying a parameter, say 'halt-on-fail' true/false, rather than let it use the default (even if that was the same as what we want). This also makes the file more explicitly clear.

jonathan1055’s picture

jonathan1055 commented

That's good. I added halt-on-fail: true temporarily to check that it works. That option does not work propery for phpcs.

Only 1 coding fault in drupalci.yml that I missed. Fixing that and setting eslint halt-on-fail to false. Can be set back to true when we have fixed the javascript problems on #3331002: [META] Javascript and YML coding standards

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes

I just made a grammar fix using the online file editor inside the MR. Not done that before. I have finished here ... ready for your final review and commit.

smustgrave’s picture

smustgrave commented
Status: Needs review » Fixed

Changes look good!

jonathan1055’s picture

jonathan1055 commented
Status: Fixed » Needs work

Thanks. The MR test ran fine, but I've just checked the newly run branch test and has ended with grey "Build sucessful", not a green "Success". However, phpunit tests have still been run. I will investigate what is going on.

jonathan1055’s picture

jonathan1055 commented

In the previous branch test log we have

---------------- Finished phpcs
...
Finished: SUCCESS

The latest one has

...
---------------- phpcs Returned 1 ----------------
---------------- Finished phpcs
...
Build step 'Execute shell' marked build as failure
Finished: FAILURE

The phpcs output is identical and the log is identical, apart from that single extra line "phpcs Returned 1". I guess this is caused by the addition of halt-on-fail: true. Even though the phpcs did not fail, it sends return code 1 which is interpreted as a failure.

jonathan1055 opened merge request !19

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Fixed

That's it! Adding 'sniff-all-files: true' to replicate the branch test showed that there was still one phpcs coding standards fault. I had already spotted this and added a patch on #3106511-26: Drupal PHP coding standards

If you commit that patch then the branch test should run and pass green, and we can leave 'halt-on-fail: true' for phpcs. The new MR on this issue does not need to be committed. I will close it.

jonathan1055 closed merge request !19

Status: Fixed » Closed (fixed)

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