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

Spelling #79

Closed
wants to merge 9 commits into from
Closed

Spelling #79

wants to merge 9 commits into from

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Jan 4, 2024

This is a follow-up to #37

Fixes misspellings identified by the check-spelling action.

The misspellings have been reported at https://github.com/jsoref/multipath-tools/actions/runs/7405969189/attempts/1#summary-20149678198

The action will report that the changes in this PR would make it happy: https://github.com/jsoref/multipath-tools/actions/runs/7405969286/attempts/1#summary-20149678465

Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
# Prefix for configfuration files (multipath.conf)
# Prefix for configuration files (multipath.conf)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is because a peer asked me about this...

@@ -187,7 +187,7 @@ The default is: \fBno\fR
.
.TP
.B multipath_dir
(Deprecated) This option is not supported any more, and the value is ignored.
(Deprecated) This option is not supported anymore, and the value is ignored.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's controversial whether "any more" is wrong, even with the meaning "no longer". The Cambridge dictionary says both spellings are possible.

Whatever.

@mwilck
Copy link
Contributor

mwilck commented Jan 5, 2024

@mwilck mwilck closed this Jan 5, 2024
@mwilck
Copy link
Contributor

mwilck commented Jan 5, 2024

I am evaluating adding your spell checking action to our workflows, so that PRs like this can be avoided in the future. Thanks!

@mwilck
Copy link
Contributor

mwilck commented Jan 5, 2024

@jsoref, this goes far beyond my knowledge about GitHub actions. I have added your file to my "tip" branch but I am getting 2453 unrecognized words. You apparently didn't need to add all these words ... what am I doing wrong?

@jsoref
Copy link
Contributor Author

jsoref commented Jan 5, 2024

@mwilck: cherry-pick jsoref@d03eaeb

Or, to answer your question: I had to configure away ~700 of the words (by excluding files, and masking parts of lines with patterns) and then add ~1800 of them. The good thing is that adding additional items to the expect file is fairly trivial, so as you go it isn't a big deal -- the workflow helps you maintain that file (and as well as excludes, and to a lesser extent patterns and the workflow itself).

@jsoref jsoref deleted the spelling branch January 5, 2024 14:43
@mwilck
Copy link
Contributor

mwilck commented Jan 5, 2024

Ok ... at first sight, your commit seems to add more patterns than actually needed.
I need to think more about this.

@bmarzins, @cvaroqui: do you reckon we should add this GitHub action and the respective code (see @jsoref's last comment above) to our repo?

@jsoref
Copy link
Contributor Author

jsoref commented Jan 5, 2024

Yes, there are a couple of boilerplate patterns that aren't truly necessary, and candidates is mostly a bootstrapping thing, although it means that if a project gains new stuff that fits a pattern it can be automatically suggested. (Note that you accidentally tagged someone else in #79 (comment))

If you're going to do this, you should probably replace @prerelease with @v0.0.22 -- prerelease has some features that I'm still working on/actively changing (especially the block-ignore feature -- I don't think it's needed for this repository).

@mwilck
Copy link
Contributor

mwilck commented Jan 5, 2024

Thanks. I'll wait for the other maintainers' thoughts.

@bmarzins
Copy link
Contributor

bmarzins commented Jan 5, 2024

I'm pretty ambivalent about this. I'm sure it will catch spelling mistakes. But I assume it will mostly just catch things that need to get added to the expect file, which at least appears easy to update. I don't see any of the boilerplate patterns hurting anything. We could always just limit spell-checking to things like the man pages, README.md, and the public library .h files. This would cut down on the busy work but still make sure the stuff visible to non-multipath-developers is spell-checked.

@jsoref
Copy link
Contributor Author

jsoref commented Jan 5, 2024

You can use only.txt to pick specific files to check (as opposed to using excludes.txt). If you have questions, I should be able to respond fairly quickly -- and if you have feedback, please feel free to send it to me.

@mwilck
Copy link
Contributor

mwilck commented Jan 9, 2024

I won't have time to work on the GH integration short-term. I will put it on a long-term todo list. Thanks to Josh's efforts, quite a few speling errors have been eliminated for now ;-)

@mwilck
Copy link
Contributor

mwilck commented Feb 9, 2024

@jsoref, I've been playing around with this a bit, strongly restricting the set of files scanned for misspellings like @bmarzins suggested. I found that check-spelling recommends adding words like the following:

+Bfile
+Bgreedy
+Bhardware
+Ioff
+Ioverrides
+Ipath

These words originate from groff escape sequences in man pages, like \fBgreedy\fR. Is there any chance to make check-spelling recognize these escape sequences, so that \fgreedy\fR is treaded as greedy and then checked? We use only \f, \n and ¸\c, so no full support for groff syntax is required 😄
Those sequences that matter most are \fB, \fI, \fP, \fR.

@jsoref
Copy link
Contributor Author

jsoref commented Feb 9, 2024

Yep, you can copy this pair of patterns:
https://github.com/check-spelling/spell-check-this/blob/9b0b6efb73b068fb39f5bb764def90742f74aab3/.github/actions/spelling/candidate.patterns#L430-L434

If you have examples beyond that set, I'm happy to adjust the thing -- certainly the patterns there have done a fairly reasonable job in my testing.

@mwilck
Copy link
Contributor

mwilck commented Feb 9, 2024

Another one

"%N:%R:%n:%r"\fR. For example: 0x200100e08ba0aea0:0x210100e08ba0aea0:.*:.* , .*:.*:iqn.2009-10.com.redhat.msp.lab.ask-06:.*

Here check-spelling sees the word aea. Can we make it recognize the entire WWNN/WWPN 0x200100e08ba0aea0:0x210100e08ba0aea0 as something that shouldn't be spell-cheked at all?

@mwilck
Copy link
Contributor

mwilck commented Feb 9, 2024

Right candidate-patterns should be cover that as well

@mwilck
Copy link
Contributor

mwilck commented Feb 9, 2024

Do I have to copy the entire candidate.patterns file or can I just pick those patterns that are relevant for us?

@jsoref
Copy link
Contributor Author

jsoref commented Feb 9, 2024

Do I have to copy the entire candidate.patterns file

No.

or can I just pick those patterns that are relevant for us?

Yes.


candidate.patterns is just a thing that will automatically suggest things, you can just copy the items you want into patterns.txt, or you can copy any portions of the file into your candidates file (merging anything you've already commented out) to get additional things it'll suggest when it runs across them.

And yes, candidates has at least one hex pattern (probably the hex/color one).

@mwilck
Copy link
Contributor

mwilck commented Feb 9, 2024

With openSUSE@391767f, I eventually got 🟢 for this workflow 😁

Patterns that might be interesting for you:

# WWNN/WWPN (NAA identifiers)
\b(?:0x)?10[0-9a-f]{14}\b
\b(?:0x|3)?[25][0-9a-f]{15}\b
\b(?:0x|3)?6[0-9a-f]{31}\b

# iSCSI iqn (approximate regex)
\biqn\.[0-9]{4}-[0-9]{2}(?:[\.-][a-z][a-z0-9]*)*\b

@jsoref
Copy link
Contributor Author

jsoref commented Feb 9, 2024

Thanks, added as check-spelling/spell-check-this@ee04617

Note that I tend to rewrite commits in @prerelease (I generally don't rewrite commits in @main unless I accidentally push... which happens when I'm working w/o sleep -- oops), so the commit sha / description may change.

--- I'll definitely be adjusting the WWNN/WWPN patterns -- probably just merging them into a single pattern as check-spelling uses the comment before a pattern when it suggests the pattern to hint to readers about its purpose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants