Improve documentation on String's methods #30148
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| /// assert_eq!(String::from_utf16(v).unwrap(), | ||
| /// "𝄞music".to_string()); | ||
| /// assert_eq!(Some(String::from("𝄞music")), | ||
| /// String::from_utf16(v)); |
There was a problem hiding this comment.
How come these assertions were swapped? I think it's generally assert(actual, expected) in terms of error messages at least.
I also think that String::from may be a bit distracting here when compared to to_string?
There was a problem hiding this comment.
I also think that String::from may be a bit distracting here when compared to to_string?
I happen to like it more in this case, as it's closely named with from_utf16.
@tbu- this isn't generic code, so .to_owned isn't appropriate.
There was a problem hiding this comment.
I can't write this test in this way anyway, as FromUtf16Error doesn't implement PartialEq, ugh
steveklabnik
force-pushed
the
doc_string
branch
from
baf16a5 to
97455b2
Compare
|
Various fixups sent! |
| /// ``` | ||
| /// let mut s = "foo".to_string(); | ||
| /// let mut s = String::from("foo"); |
There was a problem hiding this comment.
I can see how this aligns better in the from_utf16 example above, but this documentation is setting an example of how to create strings in the wild. In general although String::from is possible I would say the more idiomatic way to do this is foo.to_string(), so it seems a little odd to me that all these examples are switching to String::from?
There was a problem hiding this comment.
Right now, they're split across a lot of ways. We really need to decide what the actual convention is, there's a lot of documentation that will end up changing no matter what is chosen.
There was a problem hiding this comment.
Sure but at some point this is what's setting that convention regardless of whatever future conversations are had. It seems easier to just have less churn and not change what's there?
There was a problem hiding this comment.
Sure. Since I'm going through a sweep of std in whole, I've been trying to just do the work individually, as it's easier. Same goes for the assert order issue.
There was a problem hiding this comment.
Ah to clarify I mean leave what's here in terms of "foo".to_string()
|
@bors: r+ 97455b258fddc72942cb6dad587d8bee1c660327 Talked in person and concluded that it's probably best to land better docs for now and we can shake out conventions all at once or keep riding this train! |
|
🔒 Merge conflict |
|
☔ The latest upstream changes (presumably #30182) made this pull request unmergeable. Please resolve the merge conflicts. |
steveklabnik
force-pushed
the
doc_string
branch
from
97455b2 to
072dd6f
Compare
|
@bors: r=alexcrichton |
|
📌 Commit 072dd6f has been approved by |
Part of #29376