Bug 21892 - Robin Sheat is not a typo
Summary: Robin Sheat is not a typo
Status: CLOSED WONTFIX
Alias: None
Product: Project Infrastructure
Classification: Unclassified
Component: QA tools (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor
Assignee: Mark Tompsett
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-11-27 17:21 UTC by Mark Tompsett
Modified: 2020-06-04 20:32 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21892: Robin Sheat is not a typo [DON'T PUSH] (721 bytes, patch)
2018-11-27 17:32 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21892: Purposeful typo [DO NOT PUSH] (797 bytes, patch)
2018-11-27 17:33 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21892: Ignore Sheat when checking spelling (1.07 KB, patch)
2018-11-27 17:39 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21892: Purposefully update codespell (2.66 KB, patch)
2019-02-22 18:21 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21892: Add prompt for CodeSpell update (2.82 KB, patch)
2019-03-13 15:31 UTC, Mark Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2018-11-27 17:21:04 UTC
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.
Comment 1 Mark Tompsett 2018-11-27 17:32:57 UTC
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.
Comment 2 Mark Tompsett 2018-11-27 17:33:00 UTC
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.
Comment 3 Mark Tompsett 2018-11-27 17:39:00 UTC
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.
Comment 4 Marcel de Rooy 2018-11-28 12:37:30 UTC
Just ignore false messages? We might end up adding all names..
Comment 5 Mark Tompsett 2018-11-29 00:51:58 UTC
(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.
Comment 6 Robin Sheat 2018-11-29 12:55:53 UTC
(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.
Comment 7 Jonathan Druart 2018-12-05 21:57:12 UTC
Pushed, note that I did not get the complain with "abbout"
Comment 8 Mark Tompsett 2018-12-06 00:46:42 UTC
(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.
Comment 9 Jonathan Druart 2019-02-22 11:18:19 UTC
Tests are failing, -L does not exist in 1.6 (jessie) and 1.8 (stretch)
Comment 10 Mark Tompsett 2019-02-22 15:19:13 UTC
(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
?
Comment 11 Mark Tompsett 2019-02-22 15:56:37 UTC
(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.
Comment 12 Mark Tompsett 2019-02-22 17:43:03 UTC
(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.
Comment 13 Mark Tompsett 2019-02-22 18:21:31 UTC
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
Comment 14 Jonathan Druart 2019-03-02 17:46:28 UTC
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?
Comment 15 Mark Tompsett 2019-03-13 15:31:44 UTC
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.
Comment 16 Jonathan Druart 2019-04-29 03:04:12 UTC
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.
Comment 17 Jonathan Druart 2019-06-22 19:46:09 UTC
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
Comment 18 Magnus Enger 2019-10-03 14:05:45 UTC
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. :-)
Comment 19 Jonathan Druart 2019-10-14 13:19:33 UTC
Too much time spent on this, won't fix.
Comment 20 Robin Sheat 2019-10-14 13:51:47 UTC
:(
Comment 21 David Cook 2019-10-15 00:40:52 UTC
(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.
Comment 22 Jonathan Druart 2019-10-15 12:41:35 UTC
(In reply to Robin Sheat from comment #20)
> :(

Note that I did not mark it "invalid" ;)
Comment 23 David Cook 2019-10-17 02:01:14 UTC
(In reply to Jonathan Druart from comment #22)
> (In reply to Robin Sheat from comment #20)
> > :(
> 
> Note that I did not mark it "invalid" ;)

😂