Investigate if we need /assets/internal/.prettierrc.json

Another reason for adding our own Prettier config would be to allow a blank line in the default template

include:
  - project: $_GITLAB_TEMPLATES_REPO
    # "ref" value can be:
    # - Recommended (default) 
    # - Latest - `ref: main` 
    # - Minor or Major latests 
    # - Fixed tag - `ref: 1.0.1`
    ref: $_GITLAB_TEMPLATES_REF
    file:
      - "/includes/include.drupalci.main.yml"
      # For Drupal 7, remove the above line and uncomment the below.
      # - "/includes/include.drupalci.main-d7.yml"
      - "/includes/include.drupalci.variables.yml"
      - "/includes/include.drupalci.workflows.yml"
#      <<<=== have to add this
################

If there is no blank line we get this failure
https://git.drupalcode.org/issue/gitlab_templates-3439644/-/jobs/1302083

Another reason for having our own .prettierrc.json config might be to allow longer than 80 lines for arrays. See this commit which was needed to satisfy the formatting rules. Drupal coding standards for PHP now allow 120 chars for a single-line array before forcing it to wrap into multi-line. I don't know if there is a custom setting for YML syntax in ESLINT but it would be worth investigating.

Gitlab template's own .yml files are checked but not with the same rules/configuration as Core and Contrib. An example is our mkdocs.yml file, which I used as a starting point for contrib pages. The eslint job failed with several errors

https://git.drupalcode.org/project/scheduler/-/jobs/2767112#L32
It was easy to correct, using the download patch from the --fix option, but I think where possible this project should be using the same coding standards as Core and Contrib so that files can be copied without these problems. But prior to fixing our .yml files we need to get the intenal 'Check Code' job to detect them using the same configuration and rules.

I've pushed the first change here, to check for trailing blanks. There have been some in committed code, as metioned in the summary. There is currenty one ocurrence, which always shows as an unrelated change when using an editor which automatically removes trailing blanks. The pipeline should fail, demonstrating that this is now being checked for.

That's interesting. The ESLint job reported the error

ESlint version v8.57.1
/builds/issue/gitlab_templates-3439021/includes/include.drupalci.main.yml
  118:41  warning  Trailing spaces not allowed  no-trailing-spaces
✖ 1 problem (0 errors, 1 warning)
  0 errors and 1 warning potentially fixable with the `--fix` option.

but the Check Code job still passed green
https://git.drupalcode.org/issue/gitlab_templates-3439021/-/jobs/3216796...

That's better.
"no-trailing-spaces": 1 gives a warning, but using "no-trailing-spaces": 2 gives the error we want. EXIT_CODE is now 1 and the job fails.
https://git.drupalcode.org/issue/gitlab_templates-3439021/-/jobs/3217128...

I've taken the EXIT_CODE processing from the gitlab 'check code' job and added it to the run-local-checks script. I was running the new checks locally and saw the error warning but did not know that it would not fail the job. Replicating the process locally will save effort. Here's what you get for a failed check:

and when it passes

Now to address the question in #4. There is no reason we can't follow the proper .yml standards in mkdocs.yml. It makes sense for this project to be compliant, as it it likely that this is where other maintainers will come to look for a mkdocs.yml they can copy and modify. The latest change removes this file from the ignorePatterns: and we get simpe formatting failures. It is only single -> double quotes, and indentation faults.
https://git.drupalcode.org/issue/gitlab_templates-3439021/-/jobs/3217854

This is great progress. Happy to include the mkdocs file.

I added a .prettierrc.json into assets/internal and copied it exactly from core/.prettierrc.json in this commit

We have previously not had any Prettier config file so have been using the built-in defaults. It seems we've not been following the Core standards for quotes, see this failed job. Prettier must default to double quotes but the Core preference is for single quotes. So either we change nearly every one of our .yml files, or we deviate from Core standards and set "singleQuote": false. Which to do? one solution is trivial and easy but goes against Core. The other will involve conflicts with every other gitlab_templates MR but is the "right" thing to do, to follow Core. It's not urgent, but we do need to make a decision one way or the other.

Happy to go with the core standards. It's a good call.

Thanks. I agree. Even if it makes some conflicts in the short term it is better to follow core standards in the long run. I have made the changes for doiuble to single quotes. There will still be a couple of prettier failures - blank lines are automatically stripped at the start and end of a block, and we have no control over that rule, it is not an option. So we can remove the blank lines, add just a # at the start, or maybe find another solution.

For the empty lines, maybe we can add https://eslint.style/rules/js/no-multiple-empty-lines?

Thanks for the link, but I don't think that's the problem. It is the Prettier rules that complain, the eslint rules are fine. There were two places where we had one blank line and Prettier wanted it removed - this is not a configurable option - see https://prettier.io/docs/en/rationale#empty-lines

The first was fixed it in this commit for gitlab-ci/template.gitlab-ci.yml

The second place we had already added an unwanted comment in hidden-variables.yml and I removed it in this commit

If you are OK with these two changes then we are all sorted and this is ready for review.

I found a bug in unformatted-links.php when using it in Schedule. It required some better debug to resolve, so I left that in too.

Thanks for the extra feedback.

The changes look good. My only concern was variable replacement, but it seems that GitLab CI doesn't care whether it's single or double quotes. There is one part of the documentation where they do mention double quotes.

The downstream pipelines have run ok, so I guess all the changes work as expected. If you're happy with all the tests you've done so far, I'd say this is RTBC. The code looks good and the pipelines seem unaffected (the errors on "API" module were there already).

Given that this is quite a major change, I'd like to use this MR a little more before you merge it. Also I think the issue summary needs to add the fact of the quotes change, why we are doing it, and some links to the Prettier documentation. They have a nice rule which says "the aim is to minimise use of backslash" So you can use double quotes where the string contains a single quote. But singles are prefered if all else is equal. That needs to be said somewhere.

Ok, agree. If we can get additional testing and those minor changes in the issue description (and maybe somewhere in the documentation) it'd be great.

Regarding my comment in #3 we can allow longer lines using "printWidth": 120 in .prettierrc.json, so I have pushed that change. This length matches the Core standard for inline PHP arrays, but the YAML standard has not caught up yet (I may raise that within the Standards project). I think this change makes the variable definitions easier to read. But if you don't like it I will reverse it out.

Updated the IS with explanation about quotes.

I'm happy with the change to 120. Downstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3439021/-/pipelines/33...

RTBC unless you want to do any other improvement.

Thanks for agreeing to the 120 array line length.

The 'help us' documentation needs to be reviewed and updated, to include the bit about standards and using run-local-checks.php etc, but I will do that on 📌 Documentation pages Active .

Great, thanks so much for reviewing all the code and getting it to follow the standards close to core.
Merged.

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