Fix typos, grammar, and spelling mistakes #16273
Conversation
BenjaminBrienen
added
C-Docs
|
The generated |
rparrett
added
the
M-Needs-Migration-Guide
|
Mostly like these changes, but they should probably be held back until after the 0.15 release cycle is complete to avoid cherry-picking conflicts. I am curious as to why Quite a few breaking changes. One is a super obvious typo that should definitely get fixed up. No opinion on others. |
|
@rparrett I think I addressed all the feedback. Most of it was adding to the migration guide and applying your suggestions. Let me know if you have further comments! :) |
BenjaminBrienen
added
S-Waiting-on-Author
|
I have submitted the basic English typos to the typos CI repository. I can add the long list of exceptions to the |
There was a problem hiding this comment.
theres a lot of changes i dont like here. beyond some changes being distasteful imo, some changes are just wrong and mangle sentences. the typo fixes are good and very welcome, id hate to see them held back because of being bundled with more controversial changes
the things i dont like:
- lots of commas very liberally added that kinda hurt readability in a lot of cases
- decorating/replacing "so" with "that" and "therefore" and "thus". i dont see the point and it just reads like fluffery
- expansion of understood terms like topsorted to topologically_sorted and coeffs to coefficients, subdivs into subdivisions etc
- changing parametrize into parameterize. both are words but the latter is not the one used in this context in mathematics
- changing NdotV to N_dot_V and similar. this is an existing convention in rendering and this change has been suggested before, we stick to convention here
- im not sure changing pointee to pointed is correct. this isnt my domain but some sentences read a little weird there after the changes
- metallicity -> metalness why?
- retval -> okay why?
- birbs -> birds we're not a corporation we dont have to wear suits and ties
- zfar znear tmin tmax etc underscorification this hurts readability
i just gave this a skim, i would be very opposed to this merging as is.
id recommend making the objective uncontroversial fixes into a separate PR. id include only the following:
- a -> an changes
- words that are spelled incorrectly (e.g. extra letters, wrong letters, wrong order, wrongly spaced, stuff like strukt -> r#struct)
- conformance to american english (maths -> math, normalise -> normalize)
- dont -> don't and similar apostrophe insertions
- capitalization corrections, including pascal case such as
TestNewcomputedState=>TestNewComputedState - consistency fixes (having both coeff and coef is weird, lets stick to just one, id pick coeff)
- adding backtick code spans
- unconnected words/sentence fragments like the "You" in crates/bevy_picking/src/lib.rs line 290
i would exclude changes that expand abbreviations, rephrase comments, add commas, or change wordings, and save them for another PR.
maths -> math probably deserves its own PR even
apologies in advance if this review is overly harsh or irritating, i do appreciate the effort to fix this, i just find it a bit overzealous and opinionated
| @@ -644,7 +644,7 @@ mod tests { | |||
| let viewport_size = vec2(500., 500.); | |||
|
|
|||
| for value in (-10..10).map(|value| value as f32) { | |||
| // for a square viewport there should be no difference between `Vw` and `Vh` and between `Vmin` and `Vmax`. | |||
There was a problem hiding this comment.
"Vmax" isn't in the dictionary but "V" and "Max" are. The alternative solution is to add "Vmax" to the dictionary.
|
|
||
| # Cross-Platform Examples | ||
|
|
||
| ## 2D Rendering | ||
|
|
||
| Example | Description |
There was a problem hiding this comment.
these are a lot of changes, why add these vertical bars?
There was a problem hiding this comment.
Our markdown style is to have the tables with that style, so this is just fixing that. It is even inconsistent within the file itself.
There was a problem hiding this comment.
I can make it its own PR if you'd rather.
There was a problem hiding this comment.
does it render the same way? i think it looks noisy. can we standardize the other way?
There was a problem hiding this comment.
It does render the same way. I prefer with the bars, and I think that's what is more often used in general. I can change it if a decision-maker wants me to. We only have 2 .md files with tables.
| MorphKey::new("lshift-6", &[KeyCode::ShiftLeft], KeyCode::Digit6), | ||
| MorphKey::new("lshift-7", &[KeyCode::ShiftLeft], KeyCode::Digit7), | ||
| MorphKey::new("lshift-8", &[KeyCode::ShiftLeft], KeyCode::Digit8), | ||
| MorphKey::new("lshift-9", &[KeyCode::ShiftLeft], KeyCode::Digit9), |
There was a problem hiding this comment.
why? this probably breaks things
There was a problem hiding this comment.
"lshift" isn't in the dictionary. Should we add it to the dictionary? I'll run the example to see if it still works.
|
also baricenter is not a word, its in the typo exclusions list, not sure where in the repo it is but it should be barycenter |
|
migration guide should include maths -> math too |
I can remove the alias |
BenjaminBrienen
force-pushed
the
typos
branch
from
88e2af8 to
669d4b1
Compare
|
|
|
Will split into multiple PRs for reviewability based on the lines laid out by atlv. Thanks for the amazing feedback! :) |
| @@ -64,7 +64,7 @@ | |||
| * |output| | |||
| * | |||
| * Note that each [pass] has its own vertex and pixel shader. Remember to use | |||
| * oversized triangles instead of quads to avoid overshading along the | |||
| * oversized triangles instead of quads to avoid extra shading along the | |||
There was a problem hiding this comment.
overdraw. this is technically not precisely overdraw but its close enough. also it should say fullscreen triangle not oversized triangles (not plural)
| @@ -444,7 +444,7 @@ const SMAA_LOCAL_CONTRAST_ADAPTATION_FACTOR: f32 = 2.0; | |||
| const SMAA_AREATEX_MAX_DISTANCE: f32 = 16.0; | |||
| const SMAA_AREATEX_MAX_DISTANCE_DIAG: f32 = 20.0; | |||
| const SMAA_AREATEX_PIXEL_SIZE: vec2<f32> = (1.0 / vec2<f32>(160.0, 560.0)); | |||
| const SMAA_AREATEX_SUBTEX_SIZE: f32 = (1.0 / 7.0); | |||
| const SMAA_AREATEX_SUBTEXTURE_SIZE: f32 = (1.0 / 7.0); | |||
There was a problem hiding this comment.
this might be texel not texture. id need to read the associated papers or implementations, id leave this alone if i were you. it says TEX in a few other places and id rather stick to whatever convention the papers follow
| let mut deduplicated = component_ids.clone(); | ||
| deduplicated.sort_unstable(); | ||
| deduplicated.dedup(); | ||
|
|
||
| if deduped.len() != component_ids.len() { | ||
| if deduplicated.len() != component_ids.len() { | ||
| // TODO: Replace with `Vec::partition_dedup` once https://github.com/rust-lang/rust/issues/54279 is stabilized | ||
| let mut seen = HashSet::new(); | ||
| let mut dups = Vec::new(); | ||
| let mut duplicates = Vec::new(); | ||
| for id in component_ids { | ||
| if !seen.insert(id) { | ||
| dups.push(id); | ||
| duplicates.push(id); | ||
| } | ||
| } | ||
|
|
||
| let names = dups | ||
| let names = duplicates |
There was a problem hiding this comment.
id revert this file, dups and deduped is more readable
Objective
Code and documentation should use words that exist and are spelled correctly.
Also, I noticed a markdownlint warning to do with the table at the bottom having left and right bars (inconsistent style) so I fixed the template file.
Solution
I used the cSpell VS Code extension and literally opened every single
.rsfile to see if it popped up anycSpell"Problems". Most of them were added as "words" insettings.json. See here:Testing
The CI should be good.
Migration Guide
Color::rbga_linear->Color::rgba_linearCubicSegment.coeff->CubicSegment.coefficientsRationalSegment.coeff->RationalSegment.coefficientsRationalSegment.weight_coeff->RationalSegment.weight_coefficientsCurve::reparameterize->Curve::reparametrizeCurve::reparametrize_linear->Curve::reparameterize_linearCurve::reparametrize_by_curve->Curve::reparameterize_by_curveFallbackImageMsaa::image_for_samplecount->FallbackImageMsaa::image_for_sample_countprepare_uimaterial_nodes->prepare_ui_material_nodesGridTrack::vmin->GridTrack::v_minGridTrack::vmax->GridTrack::v_maxExitCondition::DontExit->ExitCondition::DoNotExitWinitWindows::get_fitting_videomode->WinitWindows::get_fitting_video_modeget_best_videomode->get_best_video_modeRepeatedGridTrack::vmin->RepeatedGridTrack::v_minRepeatedGridTrack::vmax->RepeatedGridTrack::v_maxmaths->math