Check for duplicates across dictionaries

[peternewman]

And no longer allow self-corrections within these checks (given we exclude them elsewhere already)

[peternewman]

This correctly catches @Gelma 's "chack" from #1478 .

It also finds some others...

They also get reported once for each dictionary file we scan after they've been found...

[peternewman]

Looks like the other one is this, not sure how we should handle that from a dictionary perspective:

codespell_lib/data/dictionary.txt:calculatble->calculatable, calculable,
codespell_lib/data/dictionary_rare.txt:calculatable->calculable

[larsoner]

@peternewman I took a stab at refactoring the code a bit to avoid double-looping (doing twice as many comparisons as necessary). I think it ends up more readable. Can you see if you agree?

[peternewman]

A few comments on your tweaks

[peternewman]

Surely now we've got your magic depends logic (which was the bit I was missing), we can just run this once to avoid duplicate errors when all the dictionaries have been added to global_err_dicts?

[larsoner]

You could but you don't need to, the depends business is just a sanity check (it should always be true based on the logic here)

[peternewman]

Ah okay. But if we did that, we'd only get the error once wouldn't we? Aren't we currently doing some sort of weird increasing square process. On2?

[larsoner]

You have to check every dict against every other dict (including itself) so you're always stuck with N * (N - 1) / 2. If you think it's clearer to move it to the other one we can, six of one half a dozen of the other to me

[peternewman]

But given we're doing it within test_dictionary_looping which gets called for each dictionary, aren't we doing it factorially, or does your global_pairs stuff not avoid this?

Materially I think we should still be able to simplify it. If we can make a dict like so:
'1nd': ['dictionary', ['1st']]
'absolete': ['dictionary', ['absolute', 'obsolete']]
'tim': ['names' ['time']]

Then when adding them, if tim is already in the dict, that means the second dict duplicates the error, and we could even give the fact the first tim came from names. Then when checking corrections, if a correction matches an error we can give the dictionary the correction and the error are in.

I guess technically we could just have the dictionary name and then the corrections in one list, but I'd imagine the code would be clearer and less confusing if they were nested arrays or hashes or whatever.

Also if we lowercase on the way in we'd remove any issues with case. Given we already check if stuff is lower-case unique within one entry I think that would be okay.

[larsoner]

But given we're doing it within test_dictionary_looping which gets called for each dictionary, aren't we doing it factorially, or does your global_pairs stuff not avoid this?

No it doesn't. On the first run, the global dict is empty so nothing is done other than comparing to itself (i.e., think of this as filling in the 0, 0 element in a matrix). Second run, the global dict has a single entry (the one from the first run), so first the 1, 1 entry is filled (second dict to itself), then 1, 0, and 0, 1. On the next, it fills the third row/col 2,0 2,1 2,2 1,2 0,2, etc.

This seems simple enough to me but if it's to complicated to understand, you can just move all the checks to the depends test, it's explicit that it iterates over exactly every pair (actually N * N not N * (N - 1) / 2 like I said previously, because it's directional) so if it's easier to "see" it there than above feel free to change it

[larsoner]

I should say no it doesn't do anything factorially. Just pairwise, as is necessary.

If you look at the global_pairs stuff it's just a sanity check -- first it checks to make sure that the given pair has not been compared, then compares them, and adds to the list of pairs that have been compared. So it ensures you only ever do N * N comparisons. And the depends test was actually added explicitly to try to make it clear that N * N pairwise comparisons had been done, and check that it's the case (though it always should be as long as nobody messes up the structure of the test)

[peternewman]

I think I got confused and thought your upper meant upper case. Not the best thing to try reviewing something complicated like this on a Night shift lunch break!

I'd assume the Python dict lookup will be slightly more efficient. More importantly now I'm all over the annotations, if we add the line number of each entry to my proposed format, we can output some errors that the annotator will catch and make it REALLY easy for people to see what errors they've made and where.

[peternewman]

This isn't doing your clever assertion error gathering either, so it currently fails on the first one, I assume, (and also says it hasn't run all the tests):
https://travis-ci.org/github/codespell-project/codespell/jobs/687269384

[larsoner]

Indeed it's not, feel free to add it if you want (it can always be added later)

[peternewman]

I'll leave it for later for now.

[peternewman]

This is now ready for review and merge if you're happy, and we can look at the other tweaks and processing in a new PR.

[larsoner]

Thanks @peternewman !