Update the wording for E0063. This will truncate the fields to 3.

[jaredwy]

Instead of listing every field it will now show missing a, z, b, and 1 other field
This is for #35218 as part of #35233

r? @jonathandturner

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[jaredwy]

Not super happy about this arm. Suggestions welcome!

[sophiajt]

I'd be tempted to use an if-expression instead.

let len = remaining_fields.len();
let truncated_fields = if len <= 3 {
        (remaining_fields.keys().take(len), "".to_string())
    } else {
        (remaining_fields.keys().take(3), format!(", and {} other field{}",
                            (len-3), if x == (len-3) {""} else {"s"}))
    };

There are probably are slicker fixes, but maybe that's an idea

[jaredwy]

This looks much cleaner :) Re compiling and merging master.

[jaredwy]

Added the r? late :( Can't remove the assignee. Sorry about that @nrc

[sophiajt]

It looks pretty good. I commented on the spot you were wondering about.

Feel free to ping me again when you want me to take another look.

[jaredwy]

@jonathandturner updated.

[sophiajt]

Great!

@bors r+ rollup

[bors]

📌 Commit 65b8be7 has been approved by jonathandturner

[jaredwy]

Well that is certainly interesting.
The order of keys in a btree isn't ensured it appears (makes sense)

@jonathandturner can i use a regex in these error strings?

[sophiajt]

@jaredwy - Instead of regex, since the label testing is substring matching, you could look for labels like "and 1 other field"

[TimNN]

@jaredwy: (I'm assuming you're referring to the travis failure)

The order of keys in a BTreeMap should be deterministic, to quote the docs "Gets an iterator over the keys of the map, in sorted order."

However from what I can tell remaining_fields is a FnvHashMap which does not guarantee any particular iteration order as far as I know.

However, even sorting by the original keys would probably not work, since the keys are names, which are just newtyped u32's indexing some table, and since that id is not deterministic as far as I know, sorting by that id would be pointless as well.

To get a deterministic sort order for the fields, the easiest solution would probably be to collect the string representation of the keys into a Vec and sort that.

(Something like let remaining_fields: Vec<_> = remaining_fields.keys().map(|n| n.as_str()).collect(); remaining_fields.sort())

Alternately do as @jonathandturner suggested and just not mention the actual field names in the error messages.

[sophiajt]

@bors r-

Pulling it out of bors for now so I can do another PR rollup. @jaredwy - if you ping me after the fix, I can put it back in the queue.

[jaredwy]

@jonathandturner I went the path of just using substring matching in the tests. The idea of using a sort meant that if anything on the type upstream to introduce stable keys changed we would lose that. The first two tests check that the keys are being outputted so i am confident we will catch a problem.

[sophiajt]

@jaredwy - SGTM. Thanks for making the update.

@bors r+ rollup

[bors]

📌 Commit 7cb1557 has been approved by jonathandturner

[jaredwy]

Looks like this has to remove all the fields. I am not in front of my computer for the weekend. I can fix up the test on monday. Might have to pull it out from the rollup. Sorry about that!

[sophiajt]

@jaredwy - no worries :) Just ping me again when you get to it.

@bors r-

[jaredwy]

@jonathandturner as per the other issue :) Been a little... preoccupied of late. This issue isn't forgotten just probably don't want code written under the influence of pain meds :D Will have the time today or over the weekend to get to this.

[bors]

☔ The latest upstream changes (presumably #36016) made this pull request unmergeable. Please resolve the merge conflicts.

[jaredwy]

I am alive and back from bed rest 👯

I went with the sort method. I wasn't happy having such a large chunk of code untested :) Alphabetical order kind of grew on me in the error output as well.

[sophiajt]

@jaredwy - welcome back!

If you think the PR is good to go, I can reapprove.

[jaredwy]

If you are happy with the alpha sorting of fields then I think it's good to go.

[sophiajt]

@jaredwy - cool with me :)

@bors r+ rollup

[bors]

📌 Commit 0e32d11 has been approved by jonathandturner