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

Add more words to the CSpell dictionary to avoid false positives. #6047

Closed
kiamlaluno opened this issue Apr 6, 2023 · 10 comments · Fixed by backdrop/backdrop#4395
Closed

Comments

@kiamlaluno
Copy link
Member

kiamlaluno commented Apr 6, 2023

Looking at the failing tests for a PR I created, I noticed that CSpell reports words that should not report, such has:

  • core/includes/bootstrap.inc:1898:21 (funcs)
    * @section sec_alt_funcs_install Use During Installation Phase
    funcs is part of a section identifier and it is fine as it is. We are certainly not going to change sec_alt_funcs_install to sec_alt_functions_install.
  • core/includes/bootstrap.inc:3357:6 (HMAC)
    * HMAC and timestamp.
    HMAC is a correct acronym. The reports about hmac are indeed correct (when hmac is not part of a function name, a variable name, or other identifiers), since it should be spelled HMAC.
  • core/includes/evalmath.inc:45:9 (funcs)
    $m->funcs()
    That is code and func() is a method. Since that is a file that comes from an external source, it is probably better to set CSpell to ignore that file.
  • core/includes/mail.inc:454:13 Unknown word (tagname)
    list($tagname) = explode(' ', strtolower($value), 2);
    That is a variable name. I guess that $tag_name would not be reported.
  • core/modules/file/file.field.inc:851:23 (hmac)
    $token = backdrop_hmac_base64($new_path,
    That is part of a function name. We are not going to change the function name to backdrop_HMAC_base64().

I understand that we want variable names to be reported, to discover code that is setting/initializing a variable that is never used again in that function/method. I also understand we want to discover lines that call a method/function that does not exist simply because the method/function name is not the correct one.
(Probably there are better tools for those cases, though.)

Still, CSpell should be set to avoid checking what follows PHPDoc tags, recognize more acronyms that are usually used in programming, and avoid all those files that we copy from an external source and for which being able to apply them a patch is more important than editing the files to fix a variable/function/method name or a comment.

@kiamlaluno
Copy link
Member Author

kiamlaluno commented Apr 6, 2023

I do not know CSpell well enough to say if it is possible to set it to ignore the identifier that follow @ㅤsection, for example, but I would suggest to avoid filling a file with comments to let CSpell ignore single lines, if possible.

@klonos
Copy link
Member

klonos commented Apr 6, 2023

Hey @kiamlaluno, I have filed a PR to address all the instances of "unknown word" mentioned in this issue (which should unblock #6019 once merged): backdrop/backdrop#4395

The PR simply adds the words HMAC (which should handle the lowercase variant as well), funcs, and tagname to the Backdrop scpell .dic file.

@klonos
Copy link
Member

klonos commented Apr 6, 2023

...I should mention that we cannot configure cspell to be ignoring variable/function/identifier names (like funcs(), sec_alt_funcs_install, or $tagname), since typos can also be made when using these throughout our code, and we need to be able to catch them.

@herbdool
Copy link

As a quick fix, I think @klonos PR will do. I added lowercase hmac and watchdog as well to your PR - hope that's okay.

Though I agree with @kiamlaluno that there are better ways to be checking typos in actual variable names and functions. Having to also update the dictionary or the code could put people off from finishing their PRs. My recent PR got bogged down with such a case. And it wasn't even code I had added, it just happened to be in the same file.

At least in my case it was a function in a test file so it could be changed, but we don't want to be forced to change public-facing functions. Or do we just accept it as normal that the PR needs to also update the dictionary if needed? We'd need to provide extra documentation and help for new contributors.

@klonos
Copy link
Member

klonos commented Apr 11, 2023

I added lowercase hmac and watchdog as well to your PR - hope that's okay.

Of course, that's absolutely fine - no need to even ask 👍🏼

Although my understanding is that the entries in the dic file are case-insensitive, so no need for both the lowercase and uppercase hmac and HMAC.

...but we don't want to be forced to change public-facing functions.

Certainly not.

Or do we just accept it as normal that the PR needs to also update the dictionary if needed?

That has been my understanding. Eventually, our codebase will have been checked and corrected (or words added to the .dic file), so these things won't be happening that often or not at all, unless the PR actually introduces typos and/or new words.

We'd need to provide extra documentation and help for new contributors.

I'm happy to either point newcomers to the right direction, or file PRs against their branches that add the words in the .dic files. That should make things easier.

@jenlampton
Copy link
Member

jenlampton commented Apr 12, 2023

It would also be nice if it didn't check the spelling in array keys (have we discussed this already?). I'm getting failures for things that shouldn't necessarily need to be spelled correctly.

@klonos
Copy link
Member

klonos commented Apr 12, 2023

@jenlampton can you please provide links to any such failures?

@klonos
Copy link
Member

klonos commented Apr 13, 2023

Although my understanding is that the entries in the dic file are case-insensitive, so no need for both the lowercase and uppercase hmac and HMAC.

@herbdool I have confirmed that ^^ with b180f7e (#4273), where I only included the lowercase version of hmac and cspell check was happy. So I've updated the PR here to remove the uppercase HMAC variant.

@backdrop/core-committers can we please merge this PR into 1.x, so that we don't have cspell failures any more? We can iterate and tweak/improve further, but as it is this will unblock many existing PRs.

@herbdool
Copy link

I've only merged into 1.x so I changed the milestone to 1.25.0.

@klonos
Copy link
Member

klonos commented Apr 14, 2023

Thanks @herbdool 🙏🏼

@jenlampton I've moved your comment re array keys in a new issue here: #6060.

@jenlampton jenlampton changed the title Set CSpell to avoid false positives Add more words to the CSpell dictionary to avoid false positives. May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants