spellcheck: complete rewrite

[alanhuang122]

I had experimented with this module a bit, and had found that I was completely unsatisfied with its performance (namely, the corrections it suggested). As a result, I rewrote the module to use aspell instead of pyenchant. The resulting performance satisfied me.

[dgw]

Please update the requirements referencing enchant to demand aspell instead. This includes requirements.txt, contrib/rpm/sopel.spec.in, & .travis.yml.

Once the requirements are up to date, Travis can test the changes. And speaking of tests, module tests come from @example decorations, which you removed in the rewrite. Please put them back so the test coverage is maintained. :)

[dgw]

@alanhuang122 When you next have time to do code-y stuff, rebasing this would be much appreciated!

[alanhuang122]

@dgw Rebased.

[dgw]

@alanhuang122 I got excited, but there's still a conflict. Did you pull master before rebasing?

Also why did AppVeyor try to build this? I haven't set that up yet lmao

[alanhuang122]

Yeahhh let's try that again.

For the record, I tried git rebase upstream/master but forgot to git fetch first.

[dgw]

That'd do it.

Self-assigned to kick the tires one last time before merging. :)

[dgw]

Hmm… Upon kicking the tires, and looking over the code once more, I have a few questions.

1. Why remove spellcheck as an alternate command name? If length is the concern, something shorter like sc can be added as an additional option (@commands('spellcheck', 'spell', 'sc').
2. Can we get a more specific command name than add for adding words to the dictionary? Something like scadd would go with the short-form I suggested in point 1.
3. Is there a way to remove custom words from the dictionary? I get that adding words is admin-only, but even a bot admin can fat-finger something and have no way to undo the mistake.

[alanhuang122]

Acknowledged, agreed, will implement.

[alanhuang122]

Re: 3: The way to remove words from the aspell dictionary is apparently just to go in and edit the file - there's no utility or function to do so from within aspell or the python bindings.

Possible solutions:

• Count on admins to not typo :v
• Maintain a staging word list in memory, and have a separate command to save words
• Have the remove command open and edit the file

The only one I don't really hate is #2.

[dgw]

#1 is what this rewrite currently does, and we seem to agree that that isn't the solution.

I don't like #3 either. Sopel shouldn't be messing with that file (or any file outside its home directory, ideally), and I have no idea if the location is predictable in all cases. What if Sopel is run as a system service user with no home directory, for example? Best to let aspell do its thing and butt out.

So… yeah, #2 is the only not-awful way to handle aspell's missing functionality.

What we'll end up with, as I understand it so far:

• spellcheck, spell, and sc as command names for checking one or more words
• scadd to add a word to the staging list
• scsave (or a better idea?) to actually call aspell's save function on each word in the staging list

Sound right? Good?

(Side rant: GitHub doesn't even allow \#1 to not reference issue/PR 1… really? No escaping?! Ugh.)

PR showing as "Approved" in the list is confusing, because we are still awaiting promised changes.

[dgw]

@alanhuang122 Did you fall off the edge of the planet world? 🔎

[dgw]

Implemented everything I asked for and more, since @alanhuang122 appears to have gone quiet.

In my opinion, I added or rewrote too much of the code to be an objective reviewer any more, so this should get a look from @Exirel, @HumorBaby, @kwaaak, and/or anyone else who's contributed recently.

[HumorBaby]

A few (minor) changes needed to ensure Python 2 support (since it is meant to be supported through this PR). One suggestion about the conversation flow.

[HumorBaby]

@dgw, A+

[dgw]

OK, now this is really "Done in Feature Rewrites" (project board status since November 2018, wow).