◐ Shell
reader mode source ↗
Skip to content

lint: add typos check#1888

Merged
Byron merged 4 commits into
gitpython-developers:mainfrom
Borda:precommit/typos
Jul 17, 2024
Merged

lint: add typos check#1888
Byron merged 4 commits into
gitpython-developers:mainfrom
Borda:precommit/typos

Conversation

@Borda

@Borda Borda commented Mar 31, 2024

Copy link
Copy Markdown
Contributor

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

@Borda Borda marked this pull request as draft March 31, 2024 20:47
@EliahKagan

EliahKagan commented Mar 31, 2024

Copy link
Copy Markdown
Member

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

EliahKagan commented Apr 1, 2024

Copy link
Copy Markdown
Member

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.

@Byron

Byron commented Apr 2, 2024

Copy link
Copy Markdown
Member

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

@Borda

Borda commented Apr 2, 2024

Copy link
Copy Markdown
Contributor Author

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 :)

@Borda

Borda commented May 7, 2024

Copy link
Copy Markdown
Contributor Author

@Borda

Borda commented May 7, 2024

Copy link
Copy Markdown
Contributor Author

@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... 🐿️

@Borda Borda marked this pull request as ready for review May 7, 2024 17:40

@Byron Byron left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide 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.

@EliahKagan EliahKagan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide 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.

@Byron Byron left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

I am now officially setting this PR to a state that indicates that some modifications are needed.

@Borda Borda force-pushed the precommit/typos branch from b1aa63d to 2ce013c Compare July 17, 2024 10:20
@Borda Borda requested review from Byron and EliahKagan July 17, 2024 10:40
@Borda

Borda commented Jul 17, 2024

Copy link
Copy Markdown
Contributor Author

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

@Byron Byron left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Thanks a lot for the minification of the PR, it looks good to me!

@Byron Byron merged commit 89822f8 into gitpython-developers:main Jul 17, 2024

@EliahKagan EliahKagan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Thanks!

@Borda Borda deleted the precommit/typos branch July 17, 2024 17:30
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Jul 24, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants