GitPython icon indicating copy to clipboard operation
GitPython copied to clipboard

lint: add typos check

Open Borda opened this issue 1 year ago • 6 comments

Just a suggestion to add a check for typos and maybe let's fix some without breaking API

Borda avatar Mar 31 '24 20:03 Borda

I've noticed a considerable number of areas in the diff where correct names are made incorrect ("rela" stands for "relative" and I don't think there are any occurrences where it should be changed to "real", and there are some others). This is not limited to the GPG signature and project-name cases that you've identified. In addition, I'm not sure any changes should be made in files in test/fixtures that are used as test repository contents.

However, I've also noticed that you've marked this as a draft, and maybe you aware of the other issues. If you think it would be helpful for me to leave a review with comments on the individual problematic cases, I'd be pleased to do so. Otherwise I will assume as long as this is a draft that such a review might be more of a distraction than a help, and refrain from it.

There are also some areas where at least the fixes are clearly a huge improvement, particularly in test/test_index.py where it had not even occurred to me that, because of the way pytest marks work, I could misspell raises and accidentally write xfail marks that don't enforce specific exception types. At minimum, that should certainly be fixed. Hopefully the idea here will work out in some form, but even if not, I can't speak for you or for Byron but from my perspective the effort so far is worth it just for finding that.

EliahKagan avatar Mar 31 '24 21:03 EliahKagan

To make sure it is not lost track of, and also to report the results of some manual testing because the affected xfail markings cover some things not produced on CI, I've opened #1893 for the bug you've discovered in test_index.py here. This is the bug where three of the xfail markings pass misspelled keyword arguments that attempt unsuccessfully to cause the test to report an unexpected failure if the exception is not as declared. It differs from some of the other misspellings found here because it affects the behavior of the tests and can cause an unexpected failure to be wrongly reported as an expected failure.

Although those are definitely not the only typos found here that should be fixed, it seems to me that their elevated importance and relationship to the correctness of the tests justifies a separate PR to fix them, especially if such a PR would result in their being fixed sooner (and then they would no longer have to be worried about here).

If you are amenable to this idea, then I suggest opening that, as you deserve the credit for it. But I would be pleased to open that PR instead if you prefer (I would list you in the Co-authored-by trailer).

While another option may be to wait for the change to come in with this PR, I think it is better that it not be delayed while figuring out if and how automated spell checking can be added safely and with an acceptably low rate of false positives.

EliahKagan avatar Apr 01 '24 02:04 EliahKagan

Thanks for sharing this draft, I am happy it could already find a genuine issue (#1893) despite a high rate of false positives. My feeling here is that given that high amount and known issues in the underlying engine, trying to make this work beyond what's here won't be worth it. But that will be for @Borda to decide, and I'd appreciate such a decision so the PR won't stay open for too long.

Thank you

Byron avatar Apr 02 '24 08:04 Byron

With my other projects, I have been using several typing tools, and this seems to be at first, lower effort, but as mentioned, it produces a significant number of false positives, and with the next version, there could be even more (just opened issues for https://github.com/crate-ci/typos/issues/966 and https://github.com/crate-ci/typos/issues/969)

So I'll open a separate PR for the fixes and most likely pivot this PR to use another typing alternative :)

Borda avatar Apr 02 '24 08:04 Borda

pivoting to https://github.com/codespell-project/codespell

Borda avatar May 07 '24 17:05 Borda

@EliahKagan @Byron, would you mind having a look at the updated version? Not sure what to do about doesnt, which is used as a file name... :chipmunk:

Borda avatar May 07 '24 17:05 Borda

@EliahKagan @Byron reverted most of my additional changes so keep it just with adding check and fixing all flagged issues, also excluding fixtures...

Borda avatar Jul 17 '24 10:07 Borda