lint: add typos check#1888
Conversation
|
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 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 |
Sorry, something went wrong.
|
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 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 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. |
Sorry, something went wrong.
|
Thanks for sharing this draft, I am happy it could already find a genuine issue (#1893) despite a high rate of false positives. Thank you |
Sorry, something went wrong.
|
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 crate-ci/typos#966 and crate-ci/typos#969) So I'll open a separate PR for the fixes and most likely pivot this PR to use another typing alternative :) |
Sorry, something went wrong.
|
pivoting to https://github.com/codespell-project/codespell |
Sorry, something went wrong.
|
@EliahKagan @Byron, would you mind having a look at the updated version? |
Sorry, something went wrong.
Byron
left a comment
There was a problem hiding this comment.
Thanks a lot for making it happen!
Now it looks like the tool is usable, and it's nice to see that it caught a couple of real errors.
I will wait for @EliahKagan approval though before merging in case I am missing some more obscure aspects of the tool and as it's integrated into the tooling of GitPython.
Sorry, something went wrong.
There was a problem hiding this comment.
I think the keyword argument spelling fixes in test/test_index.py justify adding automated spell-checking, even though various cases remain where spell-checking seems to have led to incorrect or sub-optimal changes.
These can be fixed, and the risk that spell-checking would lead to such cases being introduced later is, in my opinion, outweighed by the benefits of catching misspellings that, due to the dynamic nature of Python and its idiomatic uses, may affect the behavior of GitPython or its tests.
I've looked at each change and commented about the ones that I think should not be done or otherwise still need improvement. Some comments cover multiple changes, so the absence of a comment on a specific change does not mean that I think it is correct.
I recommend that this PR be marked as fixing #1893.
Edit: If Cygwin tests fail with "dubious ownership" errors when more commits are pushed to this pull request, that is not any fault of this PR, but also happens without the changes here. I've opened #1916 to fix it. If that pull request is merged, then merging from main or (perhaps better) rebasing this PR feature branch onto main should allow new Cygwin runs to pass here too.
Sorry, something went wrong.
Byron
left a comment
There was a problem hiding this comment.
I am now officially setting this PR to a state that indicates that some modifications are needed.
Sorry, something went wrong.
|
@EliahKagan @Byron reverted most of my additional changes so keep it just with adding check and fixing all flagged issues, also excluding fixtures... |
Sorry, something went wrong.
Byron
left a comment
There was a problem hiding this comment.
Thanks a lot for the minification of the PR, it looks good to me!
Sorry, something went wrong.
EliahKagan
left a comment
There was a problem hiding this comment.
Thanks!
Sorry, something went wrong.
Some of the CI tests use WSL. This switches the WSL distribution from Debian to Alpine, which might be slightly faster. For the way it is being used here, the main expected speed improvement would be to how long the image would take to download, as Alpine is smaller. (The reason for this is thus unrelated to the reason for the Alpine docker CI test job added in gitpython-developers#1826. There, the goal was to test on a wider variety of systems and environments, and that runs the whole test suite in Alpine. This just changes the WSL distro, used by a few tests on Windows, from Debian to Alpine.) Two things have changed that, taken together, have unblocked this: - Vampire/setup-wsl#50 was fixed, so the action we are using is able to install Alpine Linux. See: gitpython-developers#1917 (review) - gitpython-developers#1893 was fixed in gitpython-developers#1888. So if switching the WSL distro from Debian to Alpine breaks any tests, including by making them fail in an unexpected way that raises the wrong exception, we are likely to find out.
Just a suggestion to add a check for typos and maybe let's fix some without breaking API