Create downloadable patch for ESLINT automated fixes

This is great. My only worry is that few people will discover it. Oh, well, lets start here.

Thanks. I got the idea from drupalci which produced a patch for phpcs fixes. It is tedious to fix the .js and .yml standards manually if you do not have access toi run eslint locally. The initial commit has an extra call to show the diffs in the log, so that we can check the patch is the same. This can be removed in the final version, or could be left in the log as that might also alert the developer that something is available.

I need to add a logic test so that the patch is not created if there were no changes.

Two questions

1. Is there a recognised way to write a highlighted message to the log? I tried echo "*****\nThe message\n*****" but the \n does not produce a new line. Here's an example in Scheduler MR 72
2. The files that we want to write to the patch also have the mode changed. The patch contains:
   

   diff --git a/scheduler.libraries.yml b/scheduler.libraries.yml
   old mode 100644
   new mode 100755
   index dfcc17c..d24c573
   --- a/scheduler.libraries.yml
   +++ b/scheduler.libraries.yml
   @@ -1,7 +1,6 @@
    vertical-tabs:
   -  
   

   Is there a way to either avoid the mode change, or not include it in the diff output?

Fixed #5.1 by changing echo to print.

Still need to resolve #5.2 because applying the patch changes the file mode.

Also added conditional check on whether any files have actually been fixed. This works OK, and no patch is written, see this job, but we get a warning that the expected artifact file is not found.

Uploading artifacts...
junit.xml: found 1 matching artifact files and directories 
WARNING: _eslint_fixes.patch: no matching files. Ensure that the artifact path is relative to the working directory (/builds/project/scheduler) 

I assume there is a way to make an artifact optional, so we can avoid this warning.

The filemode problem is no longer a problem (I can report on that separately). Also I solved the 'optional' artifact file simply by touching the .patch file so that it exists empty with 0 bytes if there are no changes. I think this is what DrupalCI does too.

This is ready for review. Here's a log where there are eslint faults in both .js and .yml files. The changes are shown (that could be removed) but the message at the end of the log shows what is available.
https://git.drupalcode.org/project/scheduler/-/jobs/764301

Here's a job where there were no eslint faults.
https://git.drupalcode.org/project/scheduler/-/jobs/759914

One thought I had was that we could avoid the third call to eslint if there were not faults to fix. A more appropriate message could then be displayed. But for now, here is the work done, for comments/ideas/feedback

A few thoughts:
- I wouldn't call it "patch" anywhere. I would call it "diff" instead and have the extension ".diff" instead. I know that they are exactly the same, but the word "patch" carries way more meaning in our Drupal world.
- I would remove the "git diff" output, as we already have the results of the command above. The lint is already reporting the errors and giving the diff file, so there is no need to output everything. That will also make the output cleaner.
- Instead, I would probably put a one-liner on how to apply it, so people can download the diff file and then apply.

IMO patch is the right word. Diff means its a change, but patch means its a change AND its meant to be applied to the contrib project, just like patches that have traditionally been uploaded to d.o. So the meaning here matches traditional patches on d.o. IMO.

What makes you think that this patch is a confusing word?

Nothing at all. I'm totally happy with patch too. It was just to separate it from our soon-to-be-deprecated "patches", to avoid any sort of connection to the old workflow (we wouldn't want people uploading those generated patches to a d.o issue or gitlab issue). But I'm good with it too, so not a blocker at all. I that part from my comment above.

Thank you both for your comments, very informative to hear each side. I've pushed the changes as per the ammended #8

I am also going to wrap the whole 3rd eslint run and patch creation in a conditional which will not be executed if all eslint problems are already fixed. I think that question should be answers by checkig $exist_code that is stored on the 2nd run.

Here is the text at the end of the log

***********************************************************************************
The following files have some ESLINT errors and warnings which can be resolved using the --fix argument.
 
js/scheduler_default_time.js
js/scheduler_vertical_tabs.js
scheduler.libraries.yml
scheduler.links.menu.yml
scheduler_temp.js
 
A file _eslint.patch containing the changes has been created as an artifact for download.
***********************************************************************************

Is this wording OK?
Also from #8

I would probably put a one-liner on how to apply it, so people can download the diff patch file and then apply.

Do you still want that, and if so, what wording? Is git apply -v < _eslint.patch really adding anything that the maintainer/dev will not know already?

Yeah, probably not needed. That's fine.

On the last run, when it's successful, it's weird to see These are the current ESLINT errors and warnings, but then nothing listed. So maybe print that only if there are errors (inside the if).

On the last run, when it's successful, it's weird to see These are the current ESLINT errors and warnings, but then nothing listed. So maybe print that only if there are errors (inside the if).

OK. but that would mean the message coming after the output. Maybe I can swap round the order of run 1 and 2, so that we create the external junit file first and discover the $exit_code. Then the second run (writing to the log) can have a more appropriate message. I hope that the exit code will/would be the same on both runs. I will investigate and see.

I have improved the log output as requested, and as a nice side-effect it is more efficient and saves resources. The first run of eslint is now the one that creates the junit file, and this can be used to determine the exit code. Blank means there were no problems reported, so we can use this to conditionally write a messsage to the log saying 'no errors found' and avoid a second run of eslint. If there were errors or warning we now write a different message to the log and show the current messages, and then execute eslint a 3rd time with the --fix option.

There are four different scenarios that I have tested:

1. Some eslint errors are automatically fixable with with --fix, but some are not

   https://git.drupalcode.org/project/scheduler/-/jobs/790733
   MR72, via MR UI pipelines, run pipeline
   EXIT_CODE_LOG = 1, EXIT_CODE_FILE = 1, EXIT_CODE_FIX = 1

2. All eslint errors are fixable

   https://git.drupalcode.org/project/scheduler/-/jobs/790590
   MR95 via web IDE or MR 'run pipeline'
   EXIT_CODE_LOG = 1, EXIT_CODE_FILE = 1, EXIT_CODE_FIX = blank

3. No eslint errors are fixable

   https://git.drupalcode.org/project/scheduler/-/jobs/790769
   MR115 on 8.x-1.x
   $CHANGED is empty so no message about fixes
   EXIT_CODE_LOG = 1, EXIT_CODE_FILE = 1, EXIT_CODE_FIX = 1

4. There are no eslint errors or warnings in the first place

   https://git.drupalcode.org/project/scheduler/-/jobs/790784
   Scheduler branch test with _GITLAB_TEMPLATES_REPO issue/gitlab_templates-3405813, _GITLAB_TEMPLATES_REF 3405813-eslint-auto-fixes-patch
   EXIT_CODE_LOG = blank, EXIT_CODE_FILE = blank
   There is no exit_code_fix because nothing more was executed.

I tried to view the patch in the artifact browser without downloading, but I don't think we can do this, as .patch is not an allowed file extension
https://docs.gitlab.com/ee/ci/jobs/job_artifacts.html#browse-the-content...

This is ready for review

This is great! Thanks for the changes and the detailed testing. Marking RTBC.

This is a great addition. Thanks! Merged and marked as fixed.

Thank you. It started because I wanted the patch file myself :-) But then I realised if I wanted it then probably others would find it useful too.

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