While testing a bug, I changed a file with "Robin Sheat" in the comments to a .pl file, and the qa test tools flagged Sheat as a typo. It is unlikely that Sheat as a typo will be used in general comments, so the proposal is to ignore Sheat in the codespell call.
Created attachment 82680 [details] [review] Bug 21892: Robin Sheat is not a typo [DON'T PUSH] This adds a a comment to Koha.pm which should: 1) apply this patch 2) run the qa test tools -- it should fail, because it thinks Sheat is not spelled correctly.
Created attachment 82681 [details] [review] Bug 21892: Purposeful typo [DO NOT PUSH] This combined with the former patch gives an intentionally set of words incorrectly spelled. 1) apply the first two patches 2) run qa test tools -- Yes, it should find the bad spellings.
Created attachment 82682 [details] [review] Bug 21892: Ignore Sheat when checking spelling 1) without this patch applied to your qa-test-tools run the test plans for the first two patches -- both should fail, with just 1 applied, and both applied to your kohaclone. 2) apply this patch to your qa-test-tools 3) rerun the test plans for the first two patches -- just 1 applied should now pass, but both applied should still fail.
Just ignore false messages? We might end up adding all names..
(In reply to Marcel de Rooy from comment #4) > Just ignore false messages? We might end up adding all names.. If there are more names, then we could change it to -I filename.
(In reply to Marcel de Rooy from comment #4) > Just ignore false messages? We might end up adding all names.. There aren't cases as special as me.
Pushed, note that I did not get the complain with "abbout"
(In reply to Jonathan Druart from comment #7) > Pushed, note that I did not get the complain with "abbout" If you look at the default dictionary for codespell, there is a sheat entry. There is no abbout entry. Hence the lack of noise.
Tests are failing, -L does not exist in 1.6 (jessie) and 1.8 (stretch)
(In reply to Jonathan Druart from comment #9) > Tests are failing, -L does not exist in 1.6 (jessie) and 1.8 (stretch) Even after a: pip install codespell ?
(In reply to Jonathan Druart from comment #9) > Tests are failing, -L does not exist in 1.6 (jessie) and 1.8 (stretch) Had similar issues with -L not existing in my base Ubuntu Bionic. sudo apt remove codespell sudo pip uninstall codespell sudo pip install codespell All seems good.
(In reply to Jonathan Druart from comment #9) > Tests are failing, -L does not exist in 1.6 (jessie) and 1.8 (stretch) Confirmed that tests are falsely passing spelling.
Created attachment 85571 [details] [review] Bug 21892: Purposefully update codespell This detects if codespell has -L option, and if not, it purposefully uninstalls it, and then installs the version it needs via 'pip install'. It also tweaks the detection of codespell installation. Current patch-complexity: Small patch
I would not expect a script to remove/install packages without asking anything. I warning message with the detailed steps sounds better to me. What do you think?
Created attachment 86572 [details] [review] Bug 21892: Add prompt for CodeSpell update TEST PLAN --------- Now you can say no, get the spelling error. Say yes, and the spelling error goes away.
This is just complexifying everything for... what exactly? I am ok with "Sheat" being a typo, really. I am sure Robin will not mind much. I still think that display 3 lines to explain what's need to update to the new version is the best and easiest way to go.
Patch reverted. commit db31f7e3380b43ce09798b50ddc5ffd1b17b8836 Revert "Bug 21892: Ignore Sheat when checking spelling" This reverts commit 4e076f26417827494c87677b1d4f3e36ed2f2369. Tests were failing, see also bug 21892 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21892
Looks like the main patch here was pushed, then reverted. If that is correct, the status should probably not be NSO. Resetting to FQA. Feel free to correct me. :-)
Too much time spent on this, won't fix.
:(
(In reply to Marcel de Rooy from comment #4) > Just ignore false messages? We might end up adding all names.. Ignoring false messages sounds reasonable to me.
(In reply to Robin Sheat from comment #20) > :( Note that I did not mark it "invalid" ;)
(In reply to Jonathan Druart from comment #22) > (In reply to Robin Sheat from comment #20) > > :( > > Note that I did not mark it "invalid" ;) [U+1F602]