-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Clarify documentation labelling and definitions for std::collections #129866
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @workingjubilee (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
library/std/src/collections/mod.rs
Outdated
//! ### Amortized Costs | ||
//! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a subsection of the preceding section. These are not statements about conventions, these are factual statements about the collections. The introduction of sectioning here does not seem to add anything to this part of the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added subsections here originally to act as 'definition' headers, but reducing header noise is also pretty reasonable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I did see what you were trying to do! It just didn't seem to pan out quite like you intended, imo.
This comment has been minimized.
This comment has been minimized.
library/std/src/collections/mod.rs
Outdated
//! * The collection's size is denoted by `n`. | ||
//! * If a second collection is involved, it's size is denoted by `m`. | ||
//! * Item indices are denoted by `i`. | ||
//! * Operations which have an *amortized* cost are suffixed with a `*`. | ||
//! * Operations with an *expected* cost are suffixed with a `~`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of the change seems fine, so I think the conventions should just be given the list without making them into a distinct subsection.
The bots want you to fix the trailing whitespace, it seems. |
It seems you used the web interface to create this patch, which is... not the most amenable to following up on review comments, though if you want to continue working from there, that's fine by me (though you probably will have trouble if you wind up needing to rebase anything...). Let me know if you help with anything like using git on a local or running the tidy check or whatnot. |
This comment has been minimized.
This comment has been minimized.
I realised this too late as I do probably need to do some sort of a squash, I somehow imagined the web interface to be more capable than it actually is. I could reopen a new PR that is better formatted if a cleaner commit is preferred. |
Ah, no need to open a new PR. I think the GitHub Codespaces interface is slightly more capable, although I've never used it for this repo so don't trust anything I say there. In practice when I've used the web interface to create a patch and then am asked to iterate on it, what I usually have done is "just" clone the repo, add my fork as a remote, and then checkout the branch GitHub automatically created for me, and then I can just pull/push or whatever as I like. If you wind up having difficulty doing that I'm happy to do it for you once we're done with review, as long as you're okay with me making decisions about how to reorganize the info in your commits~ |
Okay I think it's ready for a review, I now understand tidy is catching the whitespace my editor was sneaking in.
For sure. |
@rustbot review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found some typos and interesting word choices.
@rustbot review |
Cool, this looks good! Want to rebase this before I send it in? |
There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged. You can start a rebase with the following commands:
The following commits are merge commits: |
Hm! |
I have pressed quite a spicy button, I think it's probably best to kill this one and reopen against master with just the changes. Edit: I can reopen if there's some way of salvaging, but my side's history is going to be somewhat tricky to untangle - I'm not going to put that mess onto anyone else to figure out. |
Page affected: https://doc.rust-lang.org/std/collections/index.html#performance Changes: - bulleted conventions - expanded definitions on terms used - more accessible language - merged Sequence and Map performance cost tables
8bfae59
to
4198594
Compare
@root-goblin This is the sequence of commands I used to take the merge commit, undo it, reset the repository state, and then rebase and push to your branch. It has been edited somewhat to remove a few back-and-forth steps, as those would have been misleading.
|
It actually only took me a couple minutes, I just didn't notice you asked for help until now because it was closed. 😅 |
Thanks for sorting it out! I'm definitely not familiar enough with this repo's commit conventions to boldly |
|
...also, while I left in a couple of instances of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
@bors r+ rollup |
I have not thought of it that way before, but yeah that makes sense. Thanks for helping out with the PR! |
Clarify documentation labelling and definitions for std::collections Page affected: https://doc.rust-lang.org/std/collections/index.html#performance Changes: - bulleted conventions - expanded definitions on terms used - more accessible language - more informative headings
…kingjubilee Rollup of 14 pull requests Successful merges: - rust-lang#129260 (Don't suggest adding return type for closures with default return type) - rust-lang#129520 (Suggest the correct pattern syntax on usage of unit variant pattern for a struct variant) - rust-lang#129696 (update stdarch) - rust-lang#129759 (Stabilize `const_refs_to_static`) - rust-lang#129835 (enable const-float-classify test, and test_next_up/down on 32bit x86) - rust-lang#129866 (Clarify documentation labelling and definitions for std::collections) - rust-lang#130052 (Don't leave debug locations for constants sitting on the builder indefinitely) - rust-lang#130077 (Fix linking error when compiling for 32-bit watchOS) - rust-lang#130123 (Report the `note` when specified in `diagnostic::on_unimplemented`) - rust-lang#130156 (Add test for S_OBJNAME & update test for LF_BUILDINFO cl and cmd) - rust-lang#130206 (Map `WSAEDQUOT` to `ErrorKind::FilesystemQuotaExceeded`) - rust-lang#130207 (Map `ERROR_CANT_RESOLVE_FILENAME` to `ErrorKind::FilesystemLoop`) - rust-lang#130219 (Fix false positive with `missing_docs` and `#[test]`) - rust-lang#130221 (Make SearchPath::new public) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#129260 (Don't suggest adding return type for closures with default return type) - rust-lang#129520 (Suggest the correct pattern syntax on usage of unit variant pattern for a struct variant) - rust-lang#129866 (Clarify documentation labelling and definitions for std::collections) - rust-lang#130123 (Report the `note` when specified in `diagnostic::on_unimplemented`) - rust-lang#130161 (refactor merge base logic and fix `x fmt`) - rust-lang#130206 (Map `WSAEDQUOT` to `ErrorKind::FilesystemQuotaExceeded`) - rust-lang#130207 (Map `ERROR_CANT_RESOLVE_FILENAME` to `ErrorKind::FilesystemLoop`) - rust-lang#130219 (Fix false positive with `missing_docs` and `#[test]`) - rust-lang#130221 (Make SearchPath::new public) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#129866 - root-goblin:patch-1, r=workingjubilee Clarify documentation labelling and definitions for std::collections Page affected: https://doc.rust-lang.org/std/collections/index.html#performance Changes: - bulleted conventions - expanded definitions on terms used - more accessible language - more informative headings
Page affected: https://doc.rust-lang.org/std/collections/index.html#performance
Changes: