Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ONGOING] CSpell: Fix nags and clean dictionary up #6302

Closed

Comments

@klonos
Copy link
Member

klonos commented Nov 25, 2023

This is a follow-up to #6255, where we've added words to our dictionary, and fixed bits of our codebase that were tripping CSpell during our automated tests. It seems that we have now gotten in a good place where there are very rare occasions where PRs fail the CSpell check.

This issue here is to serve as a place where people can report any further CSpell failures, and then to also file small follow-up PRs for these main tasks:

1. Fix any CSpell nags in new PRs

The preferred action will be based on the following logic:

  1. If the word exists only in a 3rd party library that we are using and never or very rarely change, then consider ignoring the entire file/path in the ignorePaths section of our .cspell/cspell.json configuration file. Using cspell:disable-next-line or cspell:disable/cspell:enable pairs in these cases is impractical, as we shouldn't be changing 3rd party code (we'll loose those ignores if the library is updated in the future).

  2. If the word is non-English (testing Unicode for example), the preference will be to ignore it with cspell:disable-next-line, or with cspell:disable/cspell:enable pairs when dealing with multiple lines where these words occur.

  3. If the failure is for existing variable/function names or comments, then try to fix/rename those where possible/practical instead of adding more words to our dictionary - especially when these words are used only say 2-3 times in our entire codebase.

  4. If the failure is for newly-introduced words in the PR, then:

    • try to fix those (split any combined words with dashes or underscores, or use camelCase for example)
    • or, come up with alternative words that do not trip CSpell
    • or, if the word is a made-up, fictional word used for testing purposes or as a documentation example, then see any existing fictional/test words in our dictionary can be used instead (if you are calling the thing pigglywiggly for example, then rename it to foobar which we have already added to our dictionary).
    • or, if the word occurs multiple times in a single file, but you don't want to ignore the entire file, nor use dozens of cspell:disable-next-line, then consider ignoring the specific word(s) in the specific file, by adding an entry in the overrides section of our .cspell/cspell.json configuration file.

    Again, the goal will be to avoid adding more words to our custom CSpell dictionary where possible.

  5. When fixing the codebase is not practical/possible, new words may be added to the dictionary. This will be able to happen without any core commit once we have moved our custom dictionary to its dedicated repo in https://github.com/backdrop-ops/cspell (refer to CSpell: move the backdrop.dic file to a dedicated repo under https://github.com/backdrop-ops #6280).

2. Clean up our current list of words in the custom CSpell dictionary

The biggest offenders are combined words, like thingname instead of thing_name or thing-name or thingName that are acceptable by CSpell. This is an actual list of existing words in our dictionary:

myclass
mydata
myeditor
myfield
myfragment
myfunctions
myitem
mykeyword
mynewpassword
myplugin
myprofile
mysiam
mystring
mytab
mytable
mytokens
mytype
myusername
myverylongurlexample
myverylongurl
...
newpath
newranges
newtext
nextdate
nextdatealt
nextfirst
nextsecond
...
noscheme
nosuchcolumn
nosuchindex
nosuchscheme
nosuchtable
nosuchtag
nothere
notpromoted
notpublished
...
otherjob
othername
otherval
...
somedelta
somepath
someplugin
sometable
...
testconfig
testfiles
testimage
testpage
testparam
testviews

You get the picture 😞

Lets try to rename those if they are used in a single place ("self-contained" inside the same function for example), and then remove them from the dictionary.

@bugfolder
Copy link

Getting CSpell to the point where it is adding more value than "crying wolf" has been a great accomplishment, and I wholeheartedly support the continued cleanup. It seems, then, that we should have a section in our Coding and documentation standards that mirrors the "preferred actions" above. Some of that is covered in Naming Conventions, but it would be helpful to developers to have documented the CSpell annotations to deal with things that might get flagged.

So as not to derail this issue with further discussion of the documentation topic, I've opened an issue over in the backdropcms.org issue queue: backdrop-ops/docs.backdropcms.org#231.

@klonos
Copy link
Member Author

klonos commented Nov 25, 2023

Although I never had this happen while filing all the different PRs for #6255, I always wondered how come all the // cspell:* ignore lines we have been adding were not tripping PHPCS about those comments not ending with a period. Well, I now had this come up in one of my PRs:
image

Since PHPCS is not complaining about these comments starting with a lowercase letter, I've filed a PR that only adds periods for now: backdrop/backdrop#4580

Unfortunately, PHPCS is choking on too many changes, so we won't be able to tell if it is happy now 😞 🤷🏼

@klonos
Copy link
Member Author

klonos commented Nov 25, 2023

Unfortunately, PHPCS is choking on too many changes, so we won't be able to tell if it is happy now 😞 🤷🏼

Yes we can! ...here's a test PR that only changes a single // cspell:enable line (the same one that tripped PHPCS in the other PR I posted a screenshot for in my previous comment) to add a period to it: backdrop/backdrop#4581

PHPCS didn't complain, so I guess it's just the periods at the end that are tripping it. ...we did get the usual LayoutUpgradePathTest gremlins, but nothing else failed.

@klonos
Copy link
Member Author

klonos commented Nov 25, 2023

...I've filed this issue to discuss if the cspell ignore comments should be excluded/ignored by PHPCS: backdrop-ops/phpcs#16

If that is possible, it would make things easier. Nothing to change in the way we have been adding cspell ignores.

@quicksketch
Copy link
Member

@klonos Two updates here:

I'm not sure if we want to keep this issue open, since there are now no direct action items here. Please reopen if needed.

@klonos klonos changed the title CSpell: Dictionary cleanup and follow-up nags [ONGOING] CSpell: Fix nags and clean dictionary up Jan 28, 2024
@klonos
Copy link
Member Author

klonos commented Jan 28, 2024

Thanks @quicksketch ...reopening this in order to fix more nags that have come up in recent PRs.

I'm making this an ONGOING issue.

@klonos
Copy link
Member Author

klonos commented Feb 11, 2024

I just filed a single PR to change instances of mymodue to my_module instead: backdrop/backdrop#4654

Since that has been used extensively for code examples throughout all our *.api.php files, there are plenty of lines changed over 24 files. To make it easier to review, I decided to create a single PR just for this case.

Note: Our CSpell configuration allows for camel case (for JS code and PHP class names etc.), so things like myModule or MyModuleClassXyz were not caught. MYMODULE is still caught, so I cahnged that (only a single instance of that by the way),

@quicksketch
Copy link
Member

I merged backdrop/backdrop#4935 into 1.x and 1.29.x that adds a few more words to the dictionary based on recent PRs. This set was trivial so I merged it after ensuring tests passed.

@quicksketch
Copy link
Member

I added "multipass" to the dictionary in backdrop/backdrop#4940. That seems to be the one occurring most frequently yet.

@quicksketch
Copy link
Member

I ran cspell locally to try and pin down all the remaining issues in all files. There are only a few left. I filed backdrop/backdrop#4961 which should clean up all the last known misspellings and exceptions.

@herbdool
Copy link

The changes look good to me. Knock on wood this is it for now.

@quicksketch
Copy link
Member

Huzzah! I'm going to leave it closed this time. 😀 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment