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 typos - Source code spell checker #12036

Merged
merged 18 commits into from
Feb 26, 2024
Merged

Conversation

ameknite
Copy link
Contributor

Objective

  • Avoid misspellings throughout the codebase by using typos in CI

Inspired by gfx-rs/wgpu#5191

Typos is a minimal code speller written in rust that finds and corrects spelling mistakes among source code.

  • Fast enough to run on monorepos
  • Low false positives so you can run on PRs

Solution

  • Use typos-action in CI
  • Add how to use typos in the Contribution Guide

@mockersf
Copy link
Member

prior attempt: #8324

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Build-System Related to build systems or continuous integration labels Feb 22, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Honestly I'm very impressed with the results. All of the fixes look real. How do we add false positives to an allow list?

Can you split the fixes out of this PR? I'd really like to merge them ASAP so they don't go stale: adding a spellchecker to CI is likely to be much more contentious than just fixing typos :)

@ameknite ameknite mentioned this pull request Feb 22, 2024
@ameknite
Copy link
Contributor Author

How do we add false positives to an allow list?

in typos.toml

you can add words:

[default.extend-words]
# Things that aren't typos
LOD = "LOD" # Level of detail

or just ignore them in :

extend-ignore-identifiers-re = [
  "NDK", # NDK - Native  Development Kit
]

Copy link
Contributor

@doonv doonv left a comment

Choose a reason for hiding this comment

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

Bevy has a lot more typos than I thought. This seems like a great addition to the CI!

Will fixing the typos be in the scope of this PR? I suggest you create another PR that fixes all the typos, merge that first, then merge this one.

@mockersf
Copy link
Member

Will fixing the typos be in the scope of this PR? I suggest you create another PR that fixes all the typos, merge that first, then merge this one.

#12038

typos.toml Outdated Show resolved Hide resolved
@mockersf
Copy link
Member

is it possible to have the configuration file not in the root directory?

typos.toml Outdated Show resolved Hide resolved
@ameknite
Copy link
Contributor Author

with typos-cli you can do this:
typos --config <CUSTOM_CONFIG>

but you will need to specify the path every time, I don't like it

github-merge-queue bot pushed a commit that referenced this pull request Feb 22, 2024
# Objective

Split - containing only the fixed typos

-
#12036 (review)


# Migration Guide
In `crates/bevy_mikktspace/src/generated.rs` 

```rs
// before
pub struct SGroup {
    pub iVertexRepresentitive: i32,
    ..
}

// after
pub struct SGroup {
    pub iVertexRepresentative: i32,
    ..
}
```

In `crates/bevy_core_pipeline/src/core_2d/mod.rs`

```rs
// before
Node2D::ConstrastAdaptiveSharpening

// after
Node2D::ContrastAdaptiveSharpening
```

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: James Liu <[email protected]>
Co-authored-by: François <[email protected]>
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I'm in favor of this at this point: I'm impressed with how easy this is to configure, and the very low rate of false positives.

I'm not 100% keen on adding another root-level file for this, but unless IDE support works with it elsewhere I think we should keep it. I don't mind at all having to specify a different path in CI.

However: I'd like to add this to the ci utility in the tools crate (which probably changes how the action works). Being able to invoke "check everything locally" is very helpful.

@ameknite
Copy link
Contributor Author

ameknite commented Feb 22, 2024

Every action in the ci utility uses cargo, is ok that just panic if the user doesn't have typos installed?

Idk about adding typos to the ci utility. We also don't have the toml and markdown formatting there.

And I honestly don't understand the concern of adding a new file to the root-level, isn't that just aesthetic or is there some limitation that I don't know about?

@alice-i-cecile
Copy link
Member

Idk about adding typos to the ci utility. We also don't have the toml and markdown formatting there.

Hmm okay. Leave it as is then.

And I honestly don't understand the concern of adding a new file to the root-level, isn't that just aesthetic or is there some limitation that I don't know about?

Yeah, this is just aesthetic clutter. It makes it harder to orient yourself when visiting the repo, especially for the first time. Ideally it would be in .github.

@soqb
Copy link
Contributor

soqb commented Feb 23, 2024

always wished for a cross-platform secondary default, something like project/etc/typos.toml but alas.

@ameknite ameknite force-pushed the typos branch 2 times, most recently from efe52d5 to 7565eb4 Compare February 24, 2024 01:38
Copy link
Contributor

@soqb soqb left a comment

Choose a reason for hiding this comment

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

it looks like there are still some changes to CHANGELOG.md and assets/textures/rpg/ui/**; other than that i think this is really useful.

@ameknite ameknite force-pushed the typos branch 2 times, most recently from 9ff35e4 to 8950c89 Compare February 24, 2024 19:19
@ameknite
Copy link
Contributor Author

true, I messed up.
fixed

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 24, 2024
Copy link
Member

@NiklasEi NiklasEi left a comment

Choose a reason for hiding this comment

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

This is awesome 👍

@ameknite
Copy link
Contributor Author

I'm not familair with the code in bevy_mikktspace.
"iFO",
"vOt",
"fLenOt",
idk what these variables mean so I leave them without a comment.

@alice-i-cecile
Copy link
Member

Yeah bevy_mikktspace is something of a disaster zone: it's machine-translated unsafe C code that we adopted. That's a fine solution for now.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 26, 2024
@alice-i-cecile
Copy link
Member

Merging. I'm going to leave this as an optional CI check for a while until we get a sense of the false positives and robustness (and to give older PRs a grace period where they don't need to be rebased). Once we're happy with it, I'll add it to our branch protection checks.

Merged via the queue into bevyengine:main with commit c97d010 Feb 26, 2024
25 checks passed
@alice-i-cecile alice-i-cecile mentioned this pull request Feb 26, 2024
github-merge-queue bot pushed a commit that referenced this pull request Feb 26, 2024
# Objective

The new `typos` check added in #12036 is proving its value already.
It spotted some small typos:
https://github.com/bevyengine/bevy/actions/runs/8053108519/job/21994719657?pr=12128

## Solution

Fix them.

Co-authored-by: Alice Cecile <[email protected]>
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

Split - containing only the fixed typos

-
bevyengine#12036 (review)


# Migration Guide
In `crates/bevy_mikktspace/src/generated.rs` 

```rs
// before
pub struct SGroup {
    pub iVertexRepresentitive: i32,
    ..
}

// after
pub struct SGroup {
    pub iVertexRepresentative: i32,
    ..
}
```

In `crates/bevy_core_pipeline/src/core_2d/mod.rs`

```rs
// before
Node2D::ConstrastAdaptiveSharpening

// after
Node2D::ContrastAdaptiveSharpening
```

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: James Liu <[email protected]>
Co-authored-by: François <[email protected]>
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

- Avoid misspellings throughout the codebase by using
[`typos`](https://github.com/crate-ci/typos) in CI

Inspired by gfx-rs/wgpu#5191

Typos is a minimal code speller written in rust that finds and corrects
spelling mistakes among source code.
 - Fast enough to run on monorepos
 - Low false positives so you can run on PRs


## Solution

- Use
[typos-action](https://github.com/marketplace/actions/typos-action) in
CI
- Add how to use typos in the Contribution Guide

---------

Co-authored-by: François <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Joona Aalto <[email protected]>
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

The new `typos` check added in bevyengine#12036 is proving its value already.
It spotted some small typos:
https://github.com/bevyengine/bevy/actions/runs/8053108519/job/21994719657?pr=12128

## Solution

Fix them.

Co-authored-by: Alice Cecile <[email protected]>
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

Split - containing only the fixed typos

-
bevyengine#12036 (review)


# Migration Guide
In `crates/bevy_mikktspace/src/generated.rs` 

```rs
// before
pub struct SGroup {
    pub iVertexRepresentitive: i32,
    ..
}

// after
pub struct SGroup {
    pub iVertexRepresentative: i32,
    ..
}
```

In `crates/bevy_core_pipeline/src/core_2d/mod.rs`

```rs
// before
Node2D::ConstrastAdaptiveSharpening

// after
Node2D::ContrastAdaptiveSharpening
```

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: James Liu <[email protected]>
Co-authored-by: François <[email protected]>
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

- Avoid misspellings throughout the codebase by using
[`typos`](https://github.com/crate-ci/typos) in CI

Inspired by gfx-rs/wgpu#5191

Typos is a minimal code speller written in rust that finds and corrects
spelling mistakes among source code.
 - Fast enough to run on monorepos
 - Low false positives so you can run on PRs


## Solution

- Use
[typos-action](https://github.com/marketplace/actions/typos-action) in
CI
- Add how to use typos in the Contribution Guide

---------

Co-authored-by: François <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Joona Aalto <[email protected]>
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

The new `typos` check added in bevyengine#12036 is proving its value already.
It spotted some small typos:
https://github.com/bevyengine/bevy/actions/runs/8053108519/job/21994719657?pr=12128

## Solution

Fix them.

Co-authored-by: Alice Cecile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants