tests/ui: A New Order [19/N]
#143210
tests/ui: A New Order [19/N]
#143210
Conversation
rustbot
added
S-waiting-on-review
|
I've dropped a lot of work on your head all of a sudden... Just doing my work... I hope you don't mind 😅 |
|
|
||
| let s = "\u{2a10}\u{2A01}\u{2Aa0}"; | ||
| assert_eq!(s, "⨐⨁⪠"); | ||
|
|
||
| let s = "\\{20}"; | ||
| let mut correct_s = String::from("\\"); | ||
| correct_s.push_str("{20}"); | ||
| assert_eq!(s, correct_s); |
There was a problem hiding this comment.
Good question, somehow randomly were deleted and I missed it
I'll return it
| struct NotSyncStruct { | ||
| value: isize, | ||
| } | ||
|
|
||
| impl !Sync for NotSyncStruct {} |
There was a problem hiding this comment.
NotSyncStruct -> NotSync, we avoid Hungarian Notation in Rust
| let mv = myvec(vec![1, 2, 3]); | ||
| let mv_clone = mv.clone(); | ||
| let mv_clone = myvec_deref(mv_clone); | ||
| assert_eq!(mv_clone[1], 2); | ||
| assert_eq!(myvec_elt(mv.clone()), 1); | ||
| let myvec(v) = mv; | ||
| assert_eq!(v[2], 3); | ||
| let my_vec = MyVec(vec![1, 2, 3]); | ||
| let cloned_vec = my_vec.clone(); | ||
|
|
||
| // Test extracting inner vector | ||
| let extracted = extract_inner_vec(cloned_vec); | ||
| assert_eq!(extracted[1], 2); | ||
|
|
||
| // Test getting first element | ||
| assert_eq!(get_first_element(my_vec.clone()), 1); | ||
|
|
||
| // Test direct destructuring | ||
| let MyVec(inner) = my_vec; | ||
| assert_eq!(inner[2], 3); |
There was a problem hiding this comment.
Mind keeping the original var names here? They're understandable enough so IMO not worth any churn.
There was a problem hiding this comment.
honestly, they might be undestandable, but we can do better, right?
myvec_elt for me is not understanable and fused spelling is not good here like myvec, just adding _ make it way better my_vec
There was a problem hiding this comment.
The function names indeed aren't a very clear and those changes seemed fine. I was talking specifically about variable names; mv->my_vec and mv_clone->cloned_vec doesn't really make anything more clear, so in these cases I'd lean toward preferring the existing code to keep churn down.
All the same, 🤷♂️
rustbot
added
S-waiting-on-author
It's not a problem! I think anybody can review these though, you don't need to request me in particular. (I would recommend only having two commits per PR for anyone else though so history looks final, per #143195 (comment). Jieyou may have different preferences, I know he originally requested them to be split). |
|
I thought there are people who don't like reviewing tests, and then there are people who have more important work to do, so I try not to bother them |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
I mean... we all have more to do than reviewing tests 😆 but this is a team effort. If somebody says no you can always reroll me. Btw you can about 4x the size of these PRs when you're sending them to me, no need to split things up quite so much. (I know Jieyou had other preferences here but personally I find it less of a task to review related changes together) |
Rollup of 12 pull requests Successful merges: - #136801 (Implement `Random` for tuple) - #141867 (Describe Future invariants more precisely) - #142760 (docs(fs): Touch up grammar on lock api) - #143181 (Improve testing and error messages for malformed attributes) - #143210 (`tests/ui`: A New Order [19/N] ) - #143212 (`tests/ui`: A New Order [20/N]) - #143230 ([COMPILETEST-UNTANGLE 2/N] Make some compiletest errors/warnings/help more visually obvious) - #143240 (Port `#[rustc_object_lifetime_default]` to the new attribute parsing …) - #143255 (Do not enable LLD by default in the dist profile) - #143262 (mir: Mark `Statement` and `BasicBlockData` as `#[non_exhaustive]`) - #143269 (bootstrap: make comment more clear) - #143279 (Remove `ItemKind::descr` method) Failed merges: - #143237 (Port `#[no_implicit_prelude]` to the new attribute parsing infrastructure) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #143210 - Kivooeo:tf19, r=tgross35 `tests/ui`: A New Order [19/N] > [!NOTE] > > Intermediate commits are intended to help review, but will be squashed prior to merge. Some `tests/ui/` housekeeping, to trim down number of tests directly under `tests/ui/`. Part of #133895. r? `@tgross35`
Note
Intermediate commits are intended to help review, but will be squashed prior to merge.
Some
tests/ui/housekeeping, to trim down number of tests directly undertests/ui/. Part of #133895.r? @tgross35