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 "occured" typo #99609

Closed
wants to merge 6 commits into from
Closed

Fix "occured" typo #99609

wants to merge 6 commits into from

Conversation

aaronellington
Copy link

No description provided.

@aaronellington aaronellington requested review from a team as code owners November 24, 2024 00:54
@Repiteo Repiteo added this to the 4.x milestone Nov 24, 2024
Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

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

I can't speak to whether this is a big enough change to be worth a PR, but if it is, it looks like there are several instances of changes to thirdparty files, and also a couple other changes that might need to be made.

@Repiteo
Copy link
Contributor

Repiteo commented Nov 24, 2024

I'm shocked that this wasn't caught by codespell; might need to add our own dictionary?

@tetrapod00
Copy link
Contributor

It's in the default codespell dictionary (https://github.com/codespell-project/codespell/blob/da60275a48eabf63a01c4dab6eec3c1bc55408e1/codespell_lib/data/dictionary.txt#L40933-L40933):

occured->occurred

Possibly something wrong with the codespell setup; on the docs repo it was misconfigured for a while and did not catch deeply or shallowly nested files, only files one directory deep. Or maybe it doesn't check in comments or inside snake case variable names? I'm not super familiar with codespell's limits.

@Repiteo
Copy link
Contributor

Repiteo commented Nov 24, 2024

Oh wait, the majority of changed cases were third-party files, and codespell deliberately ignores those. Must not recognize snake-case then, or it needs some special setup; not as big of an issue as I thought in either case

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Third-party files should indeed not be changed

aaronellington and others added 3 commits November 24, 2024 17:24
Co-authored-by: tetrapod <145553014+tetrapod00@users.noreply.github.com>
Co-authored-by: tetrapod <145553014+tetrapod00@users.noreply.github.com>
@aaronellington
Copy link
Author

Thanks for the feedback. I think I've made all of the requested changes. :)

@tetrapod00
Copy link
Contributor

Looks reasonable to me now, but still needs approval from the relevant maintainers

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

This makes sense, but might be better to perform in a bulk change, see:

I'll leave it up to the production team to make the final judgement if this is too small to be appropriate

@akien-mga
Copy link
Member

As suggested, this kind of changes are best consolidated together with other similar codebase cleanups, so I went ahead and did that in #99799, which includes those changes.

Thanks for your contribution!

@akien-mga akien-mga closed this Nov 28, 2024
@akien-mga akien-mga removed this from the 4.x milestone Nov 28, 2024
@adamscott
Copy link
Member

I'm shocked that this wasn't caught by codespell; might need to add our own dictionary?

We should maybe contribute it to codespell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants