core/.prettierignore and core/.prettierrc.json are not used when eslint runs in a contrib project directory, as they are not in a parent path. As a result, "prettier" eslint plugin runs in an unconfigured state, and reports bogus errors, e.g. single quotes should be changed to double quotes in .yml files: error Replace `''` with `""` prettier/prettier
Run CI for a contrib project, or manually: yarn install dependencies for core, and from a contrib project directory run ../../../core/node_modules/.bin/eslint --resolve-plugins-relative-to=../../../core .
There is already a top-level .eslintrc.json file which extends the core/.eslintrc.json file, but I don't think there's a way for a top-level .prettierrc.json to extend core/.prettierrc.json as well.
It'd be possible for drupal-core to copy the prettier config into its top-level .eslintrc.json, that way eslint would function correctly when run by contrib developers. This means there would be some redundant configuration; also is not officially recommended in case there are any prettier plugins installed that just read .prettierrc.json - see https://github.com/prettier/eslint-plugin-prettier#options
Maybe testbot could symlink core/.prettierrc.json into its parent directory? That way these configs would be in a parent directory of the directory being linted.
Note however that core/.prettierrc.json enforces ES6 javascript via the "trailingComma": "all" rule. It might be too soon to do that for any contrib projects that want to maintain IE11 support, in which case separate contrib rules with "trailingComma": "es5" should be used - see also a specific followup about this: #3314595: Core eslint forces ES6 in contrib projects.
Implement a fix and review.
None.
None.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 3314390-14.patch | 1.28 KB | mfb |
Comments
Comment #2
mfbHere's an attempted fix.
Note that this will only fix the problem for testbot!
It might be a good idea to also fix this issue for developers running core's eslint on a module in their development sandbox, i.e. drupal core itself could provide a top-level prettier config like it does for the rest of eslint.
Comment #3
mfbCorrected the command in steps to reproduce
Comment #4
jonathan1055 commentedThanks @mfb for working on this.
In the interim, until drupalci is fixed, if any contrib maintainer wants to remove the incorrect prettier coding standards faults, so that just the "real" ones are reported, you can add two files at the root of the module:
.prettierrc.jsonwhich repeats the definition from ./core/.prettierrc.json because there is no 'extends' feature, unlike in .eslintrc.json.prettierignorewhich should have *.yml as this is what .core.prettierignore hasAttached is a patch to make these. Alternatively, you can create a symbolic link via container commands in your project's drupalci.yml to mimic the link added in patch #2
Comment #5
jonathan1055 commentedSorry, unintended status change, must have been a stale form.
Comment #6
jonathan1055 commentedIn the summary you said "I don't know if there's a way for a top-level .prettierrc.json to extend core/.prettierrc.json?". I tried this in contib and the extends feature does not appear to work. It was ignored and it ran with the unconfigured eslint prettier rules.
Comment #7
mfb@jonathan1055 I am hiding your patch as it's not a patch to fix drupalci_testbot, and might confuse reviewers.
Also, fyi I don't think it should be necessary for contrib projects to have prettier ignore yml files (although of course they can if they want to). The fixes suggested by prettier are fine as far as I can tell, once the correct prettier configuration is being used.
Comment #8
mfbAdd a caveat about the trailingComma option to the issue summary.
Comment #9
jonathan1055 commentedI only did that to replicate Core. I thought that those suggestions were ignored because Core did not like them or did not want to follow them. Many of my .yml files are directly exported from drupal config UI export, and they do not follow those rules, so it makes a lot of work to change them.
But if you are saying they are good standards to follow then OK we should not ignore them. Do we have a coding standards page for .yml layout? Then we can start the discussion on how the config exports can follow the approved layout.
Comment #10
mfbDug up some background: When adding yaml linting by eslint-plugin-yml - as announced here: https://www.drupal.org/node/3226497 - prettier's yaml rules were deemed too strict and the ignore line was added - as mentioned here: https://git.drupalcode.org/project/drupal/-/merge_requests/997#note_36802
Based on this and #9 I'd say contrib modules should prettier ignore yaml files - the yaml won't be quite as "pretty" (e.g. mix of double quotes and single quotes) but at least eslint will be making sure it's valid.
Comment #11
mfbUpdated patch to also symlink the .prettierignore file
Comment #12
hestenetPatch seems good. Pushed it up to the bots: https://git.drupalcode.org/project/drupalci_testbot/-/commit/7e8bda1e8a0...
@mfb - can you confirm that the behavior is now as we want it?
Comment #13
mfbOk, looks like my adding the top-level .prettierignore symlink does nothing, because prettier does not look for one up there. It just looks in the current directory where the command is being run.
So, I guess we want to create a .prettierignore file, containing *.yml, if one doesn't already exist?
Comment #14
mfbOk here's another attempt - if it's OK to modify the project source directory, then we can write an ignore file there. But if the project already has one, then we just use that.
Comment #15
hestenetPatch 14 merged to be validated on the bots. Please review.
Comment #16
mfbRan tests on a couple projects and LGTM.
The one thing that might surprise/confuse contrib project maintainers is that testbot's eslint config is demanding ES6 javascript (since this is what core's config does). I filed #3314595: Core eslint forces ES6 in contrib projects for this - but maybe it's not necessary to worry about it at all, as of course ES5 is being phased out.
Comment #17
jonathan1055 commentedThanks @hestenet and @mfb for fixing this. Good solution and I also confirm it worked on one of my contrib projects which has no .prettierignore file.
Comment #19
jonathan1055 commentedFor reference the commit from #15 is https://git.drupalcode.org/project/drupalci_testbot/-/commit/5075fb7d421...