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 note about not being a general spell checking tool. #1535

Merged
merged 7 commits into from
Jun 13, 2021
Merged

Add note about not being a general spell checking tool. #1535

merged 7 commits into from
Jun 13, 2021

Conversation

tir38
Copy link
Contributor

@tir38 tir38 commented May 31, 2020

I spent way too much time figuring this out the hard way.

@peternewman
Copy link
Collaborator

Out of interest @tir38 what was missing to make it a general spell checking tool? What makes you think it isn't one?

@tir38
Copy link
Contributor Author

tir38 commented Jun 1, 2020

I tried to test this by just barfing rando text into a file (e.g. asdadaflkjasd) and codespell didn't report this as a mispelling

$ echo "asdfasdf" -> somefile.txt
$ echo "dne" -> someOtherFile.txt
$ codespell
./someOtherFile.txt:1: dne ==> done

@larsoner
Copy link
Member

larsoner commented Jun 1, 2020

It looks for common spelling mistakes rather than membership in a complete dictionary. Maybe it can be rephrased to take this into account?

@tir38
Copy link
Contributor Author

tir38 commented Jun 1, 2020

@larsoner yep, that was what I hoped to accomplish by updating the README

README.rst Outdated Show resolved Hide resolved
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@@ -3,6 +3,9 @@ codespell

Fix common misspellings in text files. It's designed primarily for checking
misspelled words in source code, but it can be used with other files as well.
It does not check for word membership in a complete dictionary, but instead
looks for a set of common misspellings. Therefore it might catch errors like
"adn", but it will not catch "adnasdfasdf".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Derp. Talk about circular dependency. Using Codespell to check readme of repo finds spelling error, which only exists to show example of spelling error. Not sure if I should fix this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well at least we know it's working @tir38 ! Joking apart we could either ignore the line or add the specific typo to an allowed list (although if we do the latter it would be better if it was a more obscure word/less likely typo, but perhaps that defeats the point of the example; abandonned is our goto typo, but it's not as obvious as the example one).

@larsoner Travis didn't catch this as it's only checking the module source stuff, not the surrounding files:
codespell --skip="codespell_lib/tests/test_basic.py,codespell_lib/data/*" codespell_lib/

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to add it to the checks

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done in #1596

Copy link
Collaborator

Choose a reason for hiding this comment

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

See #1535 (comment) for more detail of fixes.

README.rst Outdated Show resolved Hide resolved
@peternewman
Copy link
Collaborator

Sorry this slipped by the wayside @tir38 . I think it would still make sense to get it in.

To avoid the error it's now trapping (due to #1596), we'll need to skip it, the safest way is probably adding an exclude file into
https://github.com/codespell-project/codespell/blob/master/.github/workflows/codespell.yml
For the syntax, see:
https://github.com/codespell-project/actions-codespell#parameter-exclude_file

@peternewman peternewman reopened this Feb 21, 2021
Copy link
Collaborator

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Sorry this slipped by the wayside @tir38 . I think it would still make sense to get it in, do you want to have a go. I think the following changes should do the trick.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@@ -3,6 +3,9 @@ codespell

Fix common misspellings in text files. It's designed primarily for checking
misspelled words in source code, but it can be used with other files as well.
It does not check for word membership in a complete dictionary, but instead
looks for a set of common misspellings. Therefore it might catch errors like
"adn", but it will not catch "adnasdfasdf".

Copy link
Collaborator

Choose a reason for hiding this comment

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

See #1535 (comment) for more detail of fixes.

@peternewman peternewman added this to the v2.1.0 milestone May 28, 2021
@peternewman peternewman modified the milestones: v2.1.0, v2.2.0 Jun 10, 2021
Copy link
Contributor

@bl-ue bl-ue left a comment

Choose a reason for hiding this comment

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

🎉

@bl-ue bl-ue requested a review from peternewman June 12, 2021 16:25
@bl-ue bl-ue merged commit 8fb760b into codespell-project:master Jun 13, 2021
Copy link
Collaborator

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Sorry @bl-ue , we can't just skip the whole of README.rst because of one (deliberate) typo! How daft is codespell going to look when it ships a readme with typos in it?!?

The ideal solution is to check readme separately and ignore just the one or two deliberate typos (via ignore words). The more general solution for other people where it's less critical would be to ignoreit across the entire codebase. We should really use the ideal solution given what we're representing!

@peternewman
Copy link
Collaborator

Are you happy to re-engineer the CI tests then after this @bl-ue , or do you want me to?

@bl-ue
Copy link
Contributor

bl-ue commented Jun 14, 2021

Sorry @bl-ue , we can't just skip the whole of README.rst because of one (deliberate) typo! How daft is codespell going to look when it ships a readme with typos in it?!?

I'm sorry! That's what we get with my eager merging 😝😆

Are you happy to re-engineer the CI tests then after this @bl-ue , or do you want me to?

You mean update the CI to only ignore the specific typos in the readme? Sure, I'll do it!

@peternewman
Copy link
Collaborator

I'm sorry! That's what we get with my eager merging stuck_out_tongue_closed_eyeslaughing

😄

Are you happy to re-engineer the CI tests then after this @bl-ue , or do you want me to?

You mean update the CI to only ignore the specific typos in the readme? Sure, I'll do it!

The gold-plated solution would probably be best, skip the readme as you've done in the main run, but add another run which checks just the readme but ignores the additional deliberate typos, then we won't get "adn" all over the rest of the codebase.

@bl-ue
Copy link
Contributor

bl-ue commented Jun 14, 2021

Good idea. I'll do it 👍🏻

@tir38
Copy link
Contributor Author

tir38 commented Jun 16, 2021

thanks y'all!

@tir38 tir38 deleted the add-note-to-readme branch June 16, 2021 16:35
@bl-ue
Copy link
Contributor

bl-ue commented Jun 16, 2021

No problem @tir38 — thank you for evaluating codespell! I'd recommend some other tools that are probably more what you're thinking of when you hear spellchecker:

@peternewman
Copy link
Collaborator

No problem @tir38 — thank you for evaluating codespell! I'd recommend some other tools that are probably more what you're thinking of when you hear spellchecker:

😆 You're not supposed to plug the competition @bl-ue ! 🤣

@bl-ue
Copy link
Contributor

bl-ue commented Jun 18, 2021

Hahaha — sorry @peternewman! 😂

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

Successfully merging this pull request may close these issues.

4 participants