backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Ignore the `LICENSE.md` and `LICENSE.txt` files for CSpell. #6530

Closed avpaderno closed 6 months ago

avpaderno commented 6 months ago

CSpell checks the license files and reports spelling errors found in those files. This happened, for example, on the 4318 pull request, where CSpell reported these errors (lines from 35 to 39 included).

core/modules/ckeditor5/lib/ckeditor5/LICENSE.md:16:33 Unknown word (Wolt)
core/modules/ckeditor5/lib/ckeditor5/LICENSE.md:17:111 Unknown word (Junon)
core/modules/ckeditor5/lib/ckeditor5/LICENSE.md:19:128 Unknown word (Ashkenas)
core/modules/ckeditor5/lib/ckeditor5/LICENSE.md:23:41 Unknown word (Serhii)
core/modules/ckeditor5/lib/ckeditor5/LICENSE.md:23:48 Unknown word (Kulykov)

The license files for third-party code/libraries Backdrop uses are not supposed to be edited, not even to fix a typo in those files.

CSpell should ignore those files.

klonos commented 6 months ago

Thanks @kiamlaluno 🙏🏼 ...this makes sense, and I'm not against it, however we usually ignore the entire directory for 3rd party libraries, which would include these files anyway. Did you have any use case where licence files were flagged in a PR?

avpaderno commented 6 months ago

In one of the PRs (I do not recall which one) CSpell complained about words found in the LICENSE.md file for CKEditor 5. That is why I created this issue.

avpaderno commented 6 months ago

I found the pull request where CSpell reported errors for words contained in a license file. I added all the necessary information in the issue summary.

avpaderno commented 6 months ago

(By the way, do we have a standard way to signal an issue is about CSpell? If there is one, I would like to adhere to it.)

klonos commented 6 months ago

@kiamlaluno with https://github.com/backdrop/backdrop/pull/4737 now merged, that shouldn't be an issue. Can you please rebase the branch for that PR and confirm if the nags go away?

I still believe that if there is a 3rd party library in core, then we should be ignoring the entire thing (unless we have "forked" it and are actually responsible for maintaining the version we have in core).

As for your question with regards to whether we have any standard way to flag CSpell-related issues and PRs: nothing official (unlike the things documented in https://docs.backdropcms.org/documentation/how-to-use-the-core-issue-queue), but I believe that a CSpell: prefix should be fine ...that's what I have been doing. We usually also add the [DX] prefix for things that will make the life of developers and contributors easier.

We also have this on-going issue specifically for CSpell PRs: #6302 ...on-going issues help to reduce the amount of issues filed. How these are meant to work is that we create and merge multiple PRs under that same issue. When a PR is merged, we make sure that issue remains open - we simply adjust the has pull request and needs code review labels etc.

A final thing to keep in mind is that when the CSpell dictionary is in a good shape (not that many nags in PRs), we are planning to move it to an external repo, so that we don't have to wait for core committers to review and merge things if specific words start appearing in CSpell nags across multiple PRs. That is being tracked here if you want to follow: #6280

avpaderno commented 6 months ago

@klonos The other PR avoids that the files in the core/modules/ckeditor5/lib/ckeditor5/build directory are checked by CSpell. This PR is for any LICENSE.md or LICENSE.txt file, not just the core/modules/ckeditor5/lib/ckeditor5/LICENSE.md file, which is yet outside the core/modules/ckeditor5/lib/ckeditor5/build directory and not excluded by my previous PR.

avpaderno commented 6 months ago

I cannot yet rebase the other PR because it has been already updated yesterday. (That is why I noticed that CSpell error.)
I will update my other PR after new commits to Backdrop core, but without this PR we are not telling CSpell to ignore that LICENSE.md file; the result should not change.

klonos commented 6 months ago

...which is yet outside the core/modules/ckeditor5/lib/ckeditor5/build directory and not excluded by my previous PR.

So this is what we need to fix then. We should be excluding the entire core/modules/ckeditor5/lib/ckeditor5 directory - not just core/modules/ckeditor5/lib/ckeditor5/build. We should be making sure that we do the same with any future exclusions of 3rd party code.

klonos commented 6 months ago

...I need to restate that I am not against this change here, however, it feels like it won't be necessary if we make sure that we exclude the entire directories of any 3rd party code. There is no use case that I can think of where the licence files for 3rd party code will live outside the directory that holds the code.

avpaderno commented 6 months ago

Since core/modules/ckeditor5/lib/ckeditor5/LICENSE.md and core/modules/ckeditor5/lib/ckeditor5/build/ are the only files/directories contained in the core/modules/ckeditor5/lib/ckeditor5/ directory, I created #6536, which replaces this issue.