Capitalize all suggested fixes

[DimitriPapadopoulos]

When a wrong word is capitalized, capitalize all the suggested fixes, instead of only the first fix.

Also modify similar code, for consistency.

Fixes #2222.

[DimitriPapadopoulos]

In case of multiple suggested fixes, it might be a good idea to change misspelling.data into a list of strings early on, for example ["fix1", "fix2"], instead of a string with comma-separated values, such as "fix1, fix2".

Right now, the comma-separated string is split into a list in at least two places::

https://github.com/DimitriPapadopoulos/codespell/blob/429e472b6252d304a53355b48ae9c3bb4c50730b/codespell_lib/_codespell.py#L497

        return ", ".join(w.strip().capitalize() for w in fixword.split(","))

https://github.com/DimitriPapadopoulos/codespell/blob/429e472b6252d304a53355b48ae9c3bb4c50730b/codespell_lib/_codespell.py#L530

        opt = [w.strip() for w in misspelling.data.split(",")]

[peternewman]

Looks good @DimitriPapadopoulos .

Can we have some tests please, to cover the three cases in #1883 (comment) .

Also remembering that some custom dictionaries may still have a reason at the end (with no trailing comma):
foo->bar, baz, disabled because I decided I want to
foo->bar, baz,

[peternewman]

In case of multiple suggested fixes, it might be a good idea to change misspelling.data into a list of strings early on, for example ["fix1", "fix2"], instead of a string with comma-separated values, such as "fix1, fix2".

Sounds sensible, perhaps in a separate PR, again remember the disabled reason too.

[DimitriPapadopoulos]

I have added tests to check the case of suggested fixes, but also to check that the reason is not modified.

[peternewman]

A few suggested improvements

[peternewman]

Can we have the same test for a dictionary without a reason too.

[DimitriPapadopoulos]

Is this really useful? Good code coverage in tests should be enough, and I think we do have good coverage.

[peternewman]

I'd say yes, the no-fix format is:
adoptor->adopter, adaptor,

So we need to check it handles the trailing comma and doesn't put one on the offered suggestions for example (which a naive split/upper/join might). Plus the non-reason format is what we're actually using in our dictionaries.

[DimitriPapadopoulos]

Precisely, isn't that already checked elsewhere? In any case, such checks seem orthogonal to this PR. I mean, I don't mind adding additional checks in another PR, but I feel this PR is "hijkacked" to ask for more tests than are directly related to this PR.

[peternewman]

Precisely, isn't that already checked elsewhere? In any case, such checks seem orthogonal to this PR. I mean, I don't mind adding additional checks in another PR

I'd disagree, when this is merged, the code will be:

def fix_case(word, fixword):
    if word == word.capitalize():
        return ', '.join(w.strip().capitalize() for w in fixword.split(','))
    elif word == word.upper():
        return fixword.upper()
    # they are both lower case
    # or we don't have any idea
    return fixword

By observation, if fixword was passed in with the trailing comma, then capital case would either strip it off, or return an extra space on the end of the line, but upper and unmodified would pass it back out unmodified. i.e. the behaviour varies between the modes, which I'd say means we should test it while we're touching this function.

I've checked the other code and it does currently strip the trailing comma with no reason, but we've got no guarantee of that in this code.

"hijkacked"

Can you add hijkacked->hijacked ? 😆

[DimitriPapadopoulos]

I see. I do know that this function does not get trailing commas currently, but I understand there's no guarantee this won't happen in the future.

I can add the test, but it would be better spending time on changing fixword to be a real list before it gets to this function. But then we would need to make sure the code changing the comma-separated words from the dictionary into a list does the right thing. However, I feel that would be better handled in unit tests.

[DimitriPapadopoulos]

How does it look like now?

[peternewman]

Very belated, but LGTM thanks @DimitriPapadopoulos

[peternewman]

In case of multiple suggested fixes, it might be a good idea to change misspelling.data into a list of strings early on, for example ["fix1", "fix2"], instead of a string with comma-separated values, such as "fix1, fix2".

Yeah, or call the fixcase function on each word, but that might not be as efficient.

Right now, the comma-separated string is split into a list in at least two places::