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

Non-breaking change* from UK spellings to US #8291

Merged
merged 17 commits into from
Apr 8, 2023

Conversation

BlackPhlox
Copy link
Contributor

@BlackPhlox BlackPhlox commented Apr 2, 2023

Fixes issue mentioned in PR #8285.

Note: By mistake, this is currently dependent on #8285

Objective

Ensure consistency in the spelling of the documentation.

Exceptions:
crates/bevy_mikktspace/src/generated.rs - Has not been changed from licence to license as it is part of a licensing agreement.

Maybe for further consistency, https://github.com/bevyengine/bevy-website should also be given a look.

Solution

Changed the spelling of the current words (UK/CN/AU -> US) :

cancelled -> canceled (Breaking API changes in #8285)
behaviour -> behavior (Breaking API changes in #8285)
neighbour -> neighbor
grey -> gray
recognise -> recognize
centre -> center
metres -> meters
colour -> color

Update [engine_style_guide.md] Moved to #8324


Changelog

Changed UK spellings in documentation to US

Migration Guide

Non-breaking changes*

* If merged after #8285

@BlackPhlox BlackPhlox changed the title Changed UK spellings to US Non-breaking change* from UK spellings to US Apr 2, 2023
@BlackPhlox BlackPhlox marked this pull request as ready for review April 2, 2023 08:52
@james7132 james7132 added the C-Code-Quality A section of code that is hard to understand or change label Apr 2, 2023
cart pushed a commit that referenced this pull request Apr 5, 2023
# Objective

In the
[`Text`](https://github.com/bevyengine/bevy/blob/3442a13d2cb4b2d77c9f7bf8b6dad25742bd21d7/crates/bevy_text/src/text.rs#L18)
struct the field is named: `linebreak_behaviour`, the British spelling
of _behavior_.
**Update**, also found: 
- `FileDragAndDrop::HoveredFileCancelled` 
- `TouchPhase::Cancelled`
- `Touches.just_cancelled`

The majority of all spelling is in the US but when you have a lot of
contributors across the world, sometimes
spelling differences can pop up in APIs such as in this case. 

For consistency, I think it would be worth a while to ensure that the
API is persistent.

Some examples:
`from_reflect.rs` has `DefaultBehavior`
TextStyle has `color` and uses the `Color` struct.
In `bevy_input/src/Touch.rs` `TouchPhase::Cancelled` and _canceled_ are
used interchangeably in the documentation

I've found that there is also the same type of discrepancies in the
documentation, though this is a low priority but is worth checking.
**Update**: I've now checked the documentation (See #8291)

## Solution

I've only renamed the inconsistencies that have breaking changes and
documentation pertaining to them. The rest of the documentation will be
changed via #8291.

Do note that the winit API is written with UK spelling, thus this may be
a cause for confusion:
`winit::event::TouchPhase::Cancelled => TouchPhase::Canceled`
`winit::event::WindowEvent::HoveredFileCancelled` -> Related to
`FileDragAndDrop::HoveredFileCanceled`

But I'm hoping to maybe outline other spelling inconsistencies in the
API, and maybe an addition to the contribution guide.

---

## Changelog

- `Text` field `linebreak_behaviour` has been renamed to
`linebreak_behavior`.
- Event `FileDragAndDrop::HoveredFileCancelled` has been renamed to
`HoveredFileCanceled`
- Function `Touches.just_cancelled` has been renamed to
`Touches.just_canceled`
- Event `TouchPhase::Cancelled` has been renamed to
`TouchPhase::Canceled`

## Migration Guide

Update where `linebreak_behaviour` is used to `linebreak_behavior`
Updated the event `FileDragAndDrop::HoveredFileCancelled` where used to
`HoveredFileCanceled`
Update `Touches.just_cancelled` where used as `Touches.just_canceled`
The event `TouchPhase::Cancelled` is now called `TouchPhase::Canceled`
@BlackPhlox BlackPhlox marked this pull request as draft April 6, 2023 09:57
@BlackPhlox BlackPhlox marked this pull request as ready for review April 6, 2023 11:38
@JoJoJet JoJoJet added the C-Docs An addition or correction to our documentation label Apr 6, 2023
@james7132 james7132 added the X-Controversial There is active debate or serious implications around merging this PR label Apr 6, 2023
@james7132
Copy link
Member

Marking this as controversial due to updating project guidelines. To make sure this is a direction we want to go in.

@james7132 james7132 requested review from cart and alice-i-cecile April 6, 2023 17:32
@alice-i-cecile
Copy link
Member

I'm opposed to enforcing this without an automated way to check this.

@BlackPhlox
Copy link
Contributor Author

BlackPhlox commented Apr 6, 2023

I'm opposed to enforcing this without an automated way to check this.

@alice-i-cecile Working on it 🚀 #https://github.com/BlackPhlox/bevy/pull/1

@alice-i-cecile
Copy link
Member

If we can get this in CI without massive false positives I'm on board with it for consistency :) Ideally we're also checking type and method names too?

I think that rather than using a generic spellchecker, we should probably consider a blacklist approach? Software dev always has a ton of "unusual" words, and game dev is even worse for that.

@BlackPhlox
Copy link
Contributor Author

BlackPhlox commented Apr 6, 2023

@alice-i-cecile Agreed! I think it might create too much friction for everyone involved if it is a whitelist. So I think we should wait until blacklist functionality has been developed for check-spelling, see check-spelling - Feature : Dictionary deltas. So currently I'm just testing by compiling my own whitelist just to see if it would work from a CI standpoint. See latest CI run for how it looks currently. (Note: the gh action does have an accompanying bot)

@BlackPhlox
Copy link
Contributor Author

BlackPhlox commented Apr 7, 2023

@alice-i-cecile I've updated my opinion on this and reflected my concerns in #8324. I think we should continue the discussion to that pr instead. Maybe then I should remove engine_style_guide.md changes from this pr, so this pr is trivial instead of marked controversial and do the change in #8324 instead?

@alice-i-cecile
Copy link
Member

Agreed on moving that, and removing the style guide changes :)

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed X-Controversial There is active debate or serious implications around merging this PR labels Apr 8, 2023
@alice-i-cecile
Copy link
Member

Canadian spelling are obviously the superior option, but I definitely think we should standardize, even if informally.

Merging now; we can continue the discussion on automated tools to enforce this.

@alice-i-cecile alice-i-cecile enabled auto-merge April 8, 2023 00:39
auto-merge was automatically disabled April 8, 2023 01:09

Head branch was pushed to by a user without write access

@JoJoJet
Copy link
Member

JoJoJet commented Apr 8, 2023

I don't agree that style guide changes need to be accompanied by automated enforcement. They're just guidelines, and I think that suggesting a standard style is better than being silent on it. Plus, international spellings will most likely slip past code review in the future, so having this in the style guide would make it easier for contributors to write a PR to fix them (gives them an easy motivation to refer to, which can be the scariest part of making a PR for first-time contributors).

That said, the style guide changes should probably be in a separate PR.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 8, 2023
Merged via the queue into bevyengine:main with commit e931225 Apr 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Code-Quality A section of code that is hard to understand or change C-Docs An addition or correction to our documentation 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.

4 participants