Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add codespell: config + action (to detect new typos) and make it fix a good number of them #11195

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

yarikoptic
Copy link
Contributor

@yarikoptic yarikoptic commented Mar 7, 2024

codespell was used occasionally on some files, so not a new tool here. But now it would guard RTD from having typos introduced to begin with.

Note that some fixes could affect (fix) functionality.

There is an odd "variable" syntaxt which I didn't fix since seems would require transition and smells like it is on purpose:

❯ git grep syntaxt -- readthedocs/
readthedocs/projects/migrations/0106_add_addons_config.py:                ("syntaxt", models.CharField(max_length=128)),
readthedocs/projects/models.py:    syntaxt = models.CharField(max_length=128)
readthedocs/projects/tasks/builds.py:        grab them by using glob syntaxt between other files that could be garbage.

or was it intended to be syntax ?

TODOs


📚 Documentation previews 📚

Copy link

sentry-io bot commented Mar 7, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: readthedocs/api/v2/views/integrations.py

Function Unhandled Issue
get_closed_external_version_response TypeError: 'NoneType' object is not subscriptable /api/v2/webhook/{project_slug}/{integration_pk}...
Event Count: 1

Did you find this useful? React with a 👍 or 👎

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 👍🏼

I didn't take a full review, but I noted a few issues that I see will generate some conflicts. In particular, those that change Python code and require some manual interaction (e.g. a Django migration to update the help_text)

I'm not opposed to this work, but I find it hard to prioritize by the core team at this point.

@@ -66,7 +66,7 @@ class Migration(migrations.Migration):
"action_arg",
models.CharField(
blank=True,
help_text="Value used for the action to perfom an operation",
help_text="Value used for the action to perform an operation",
Copy link
Member

Choose a reason for hiding this comment

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

This would require a Django migrations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, let me exclude all such lines with help_text *=.*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

even better -- while reviewing tuneups needed for pre-commit (e.g. #11201 and from there on) - ralized that it is migrations -- better be excludes altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry for the noise -- original is still in readthedocs/builds/models.py . I will add explicit skip for this phrase and TODO

Copy link
Member

Choose a reason for hiding this comment

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

Note we can't skip the whole migrations directory since we are running safemigrations pre commit over it mainly, but also other checks as well

@@ -170,7 +170,7 @@ Version 10.15.1
* `@stsewd <https://github.com/stsewd>`__: Update docs requirements (`#11032 <https://github.com/readthedocs/readthedocs.org/pull/11032>`__)
* `@github-actions[bot] <https://github.com/github-actions[bot]>`__: Dependencies: all packages updated via pip-tools (`#11029 <https://github.com/readthedocs/readthedocs.org/pull/11029>`__)
* `@stsewd <https://github.com/stsewd>`__: Redirects: limit to 100 per project (`#11028 <https://github.com/readthedocs/readthedocs.org/pull/11028>`__)
* `@humitos <https://github.com/humitos>`__: Build: reset notifications when reseting a build (`#11027 <https://github.com/readthedocs/readthedocs.org/pull/11027>`__)
* `@humitos <https://github.com/humitos>`__: Build: reset notifications when resetting a build (`#11027 <https://github.com/readthedocs/readthedocs.org/pull/11027>`__)
Copy link
Member

Choose a reason for hiding this comment

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

I think there is no need to touch this file. Also, changing the name of the PRs here won't change them in GH and these won't match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my 1c: IMHO it is worth fixing it as it is "user facing" , whenever PRs with their titles -- developer facing etc.

But if you insist - I can easily skip it. Just one more call ;)

.github/workflows/codespell.yml Outdated Show resolved Hide resolved
.github/workflows/codespell.yml Outdated Show resolved Hide resolved
@agjohnson
Copy link
Contributor

We use pre-commit for all other linting checks. I feel if we implement this it should be at pre-commit, not GHA.

@yarikoptic
Copy link
Contributor Author

Hmm, interesting, my script should've detected presence of pre-commit config... Will have a look wherever get a chance. You do have ci job running the pre-commit I assume? (I don't see report from pre-commit service)

@yarikoptic
Copy link
Contributor Author

that explains it:

❯ ls -ld .pre-commit-config.yaml
lrwxrwxrwx 1 yoh yoh 29 Mar  7 18:00 .pre-commit-config.yaml -> common/pre-commit-config.yaml
❯ cat .pre-commit-config.yaml
cat: .pre-commit-config.yaml: No such file or directory
❯ git submodule
-4af0fffd2cbeeb40f0a71b875beb99d6dc88a355 common

Let's do the submodules dance now...

=== Do not change lines below ===
{
 "chain": [],
 "cmd": "codespell -w -i 3 -C 2",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
=== Do not change lines below ===
{
 "chain": [],
 "cmd": "codespell -w",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants