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

Fix typos #6643

Merged
merged 1 commit into from
Apr 7, 2023
Merged

Fix typos #6643

merged 1 commit into from
Apr 7, 2023

Conversation

wutchzone
Copy link
Contributor

Hello,
I have run the spellchecker across the whole codebase and found some typos.

@pascalkuthe
Copy link
Member

pascalkuthe commented Apr 7, 2023

I have been thinking about doing something similar with https://github.com/crate-ci/typos (which fixes all typos it finds) and then running it in CI but the initial typo PR would break a lot of PRs so I am not sure if it's worth it.

Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

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

Thanks.

@wutchzone
Copy link
Contributor Author

@pascalkuthe I have experimented with this idea of adding typos to the CI in some of my projects. However, I found it quite frustrating because it can detect many false positive results. I think it can only work in the CI if the detected errors of the typos are not hard errors but rather warnings.

... running it in CI, but the initial typo PR would break a lot of PRs so I am not sure if it's worth it.

I think this is not a concern because it should be easily rebase-able. I think the main problem is to decide which files should be checked and which not. For example the CHANGELOG.md, you do not want to correct previous errors in previous releases, but only the new ones, because the changelog should be only "appendable". However, the typos can exclude only whole files and not the sections of the file.

However if you want I can draft a PR, where we can experiment with the typos in the CI.

@pascalkuthe
Copy link
Member

@pascalkuthe I have experimented with this idea of adding typos to the CI in some of my projects. However, I found it quite frustrating because it can detect many false positive results. I think it can only work in the CI if the detected errors of the typos are not hard errors but rather warnings.

The reason I wanted it in CI is that I want to use typos locally to check/fix my commits but if there are existing errors it will endup generating a bunch of unrelated changes to existing code which is unacceptable. It's pretty straightforward to add exceptions to typos so it might still be worth it.

I think the main problem is to decide which files should be checked and which not. For example the CHANGELOG.md, you do not want to correct previous errors in previous releases, but only the new ones, because the changelog should be only "appendable". However, the typos can exclude only whole files and not the sections of the file.

I think retroactively fixing typos in the CHANGELOG is quite alright. But we have a ton of testcases in helix which consist of gibberish text (like fooobar). I don't want to get typo errors for those so that's a bit more challenging.

I think this is not a concern because it should be easily rebase-able.

Not quite if you rename a variable (by fixing a typo) and somebody uses that variable in their code then no conflict will show up and CI won't fail until we actually merge. We have almost 300 open PRs that's why we are usually pretty careful about renames for that reason.

@pickfire
Copy link
Contributor

pickfire commented Apr 7, 2023

I think right now typos have too many false positive in tests, maybe we might want to exclude some stuff, but more of an issue now I think is for themes and language support, that one takes more time to review and is less automated now.

We can add typos in another pull request but I will merge this for now. I think typo fix is still fine, but at least not changing name for not much benefit.

@pickfire pickfire merged commit e856906 into helix-editor:master Apr 7, 2023
Triton171 pushed a commit to Triton171/helix that referenced this pull request Jun 18, 2023
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
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