Skip to content
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

Updating str.chars docs to mention crates.io. #68495

Merged
merged 1 commit into from
Feb 17, 2020
Merged

Updating str.chars docs to mention crates.io. #68495

merged 1 commit into from
Feb 17, 2020

Conversation

thesoftwarephilosopher
Copy link
Contributor

This might spare someone else a little time searching the stdlib for unicode/grapheme support.

This might spare someone else a little time searching the stdlib for unicode/grapheme support.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 23, 2020
@thesoftwarephilosopher
Copy link
Contributor Author

Thanks for the info robot.

@steveklabnik
Copy link
Member

We generally don't write docs like this; I will let @rust-lang/libs decide if they want this or not, however.

@steveklabnik
Copy link
Member

(For slightly more context, negative statements in docs tend to go out of date quickly; if we did add this functionality, this comment would very likely get out of date. Additionally, we don't generally pick favorites, and so mentioning crates.io by name is also unusual.)

@thesoftwarephilosopher
Copy link
Contributor Author

(For slightly more context, negative statements in docs tend to go out of date quickly; if we did add this functionality, this comment would very likely get out of date. Additionally, we don't generally pick favorites, and so mentioning crates.io by name is also unusual.)

The only reason I even made this PR was after reading the whole thread at https://users.rust-lang.org/t/how-do-you-iterate-over-grapheme-clusters-of-a-string-in-rust/11338/3

@thesoftwarephilosopher
Copy link
Contributor Author

We generally don't write docs like this; I will let @rust-lang/libs decide if they want this or not, however.

Up to you guys, I have no preference. Just tried to spare someone else a little lost time.

@LukasKalbertodt
Copy link
Member

I'm in favor of adding this information. I wouldn't even mind mentioning the unicode-segmentation crate specifically. Functionality for unicode grapheme clusters were deprecated and then removed due to the large required unicode tables. The deprecation PR even specifically mentions three replacement crates (including unicode-segmentation). How about something like this?

Iteration over grapheme clusters may be what you actually want. This functionality is not provided by Rust's standard library, however. Instead, you can use external crates, like unicode-segmentation on crates.io.


we don't generally pick favorites, and so mentioning crates.io by name is also unusual.

While I agree that we shouldn't pick favorites, I think it's ok to mention crates as examples. Also, the documentation of std already mentions crates.io and specific crates in some places:

For slightly more context, negative statements in docs tend to go out of date quickly; if we did add this functionality, this comment would very likely get out of date.

I don't think that is a problem in this case. If grapheme cluster iteration would be added, the docs of str::chars would be one of the first things coming to my mind that would need adjustments.

@Dylan-DPC-zz
Copy link

@sfackler @steveklabnik you fine with this?

@Mark-Simulacrum
Copy link
Member

I would propose that we update the note as @LukasKalbertodt suggested, including the unicode-segmentation crate. However, it may make sense to ask @BurntSushi - who I feel like probably knows the most about this, though I'm not sure why I think that :) - what they think about this too.

r? @BurntSushi

@rust-highfive rust-highfive assigned BurntSushi and unassigned sfackler Feb 16, 2020
@BurntSushi
Copy link
Member

@Mark-Simulacrum I'm not quite sure I know the most about segmentation, but I have some experience with it. In particular, the bstr crate has an alternative implementation of grapheme iteration that works on &[u8] and can be faster than the unicode-segmentation crate. (Because it uses a DFA. This also means the implementation is considerably simpler if you exclude the complexity of DFA generation.)

But I think this probably deserves a more formal policy decision from the libs team. I don't know if this PR is the right place to put it. If I put my Rust user hat on, then yes, I would very much like to see crate recommendations in std itself. They are nice pointers and will carry a lot of weight towards blessing certain crates. But if I put my library team hat on, I somewhat come down on the side of @steveklabnik here. We have generally been very careful not to bless specific crates in official documentation. IMO, if we are going to reverse that, then there should be some explicit decision to do so, and it probably should not occur piecemeal on PRs like this. (I do have some bias here in this particular example because I just so happen to maintain an alternative crate, but I am genuinely trying to speak more generally here.)

And yes, I do see that we do have specific crate recommendations in a few spots already. IMO, we should not jump so quickly to consider these as solid precedent for continuing to mention crates in documentation. It's much more likely that the existing recommendations slipped in because they escaped review. Which is understandable, because it's very easy to see such things as an unquestionably good idea.

Anyway, I guess my bottom line is that I'm not necessarily opposed to blessing specific crates in official docs, but rather, think it should be a decision made via a more explicit discussion. I'm not sure if it rises to the level of an RFC though.

I am okay with mentioning crates.io in official docs though.

@Mark-Simulacrum
Copy link
Member

Hm, okay, I had thought that there was pretty much only one option in the ecosystem, but it sounds like that's not the case (and I guess in any case that would likely get rapidly stale).

I agree that trying to decide whether (and how) to incorporate crates.io "recommendations" should not be made here. I'm also not sure if it rises to the level of an RFC, but I think that is probably the right way to go -- at least, I don't know of any other mechanism we have in between. I don't personally think FCP would be appropriate here due to the weight of this specific issue.

Given that this PR's current state is just indicating that the functionality is available on crates.io, without mentioning a specific crate, let's merge that in. If folks are interested in further discussing this change, then I think a pre-RFC and so forth is probably the way to go... or at least, is a good start.

@bors r+

If I put my Rust user hat on, then yes, I would very much like to see crate recommendations in std itself. They are nice pointers and will carry a lot of weight towards blessing certain crates.

I guess one other alternative is to mention the cookbook in the string documentation (https://rust-lang-nursery.github.io/rust-cookbook/text.html) and link to that more generally. I'm not sure how well maintained it is these days though.

@bors
Copy link
Contributor

bors commented Feb 16, 2020

📌 Commit ac19dff has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 16, 2020
Updating str.chars docs to mention crates.io.

This might spare someone else a little time searching the stdlib for unicode/grapheme support.
@bors
Copy link
Contributor

bors commented Feb 16, 2020

⌛ Testing commit ac19dff with merge a0af395d7ab45de7e964eb8eb76062988a137a38...

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 16, 2020
Updating str.chars docs to mention crates.io.

This might spare someone else a little time searching the stdlib for unicode/grapheme support.
@JohnTitor
Copy link
Member

@bors retry rolled up

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 16, 2020
Updating str.chars docs to mention crates.io.

This might spare someone else a little time searching the stdlib for unicode/grapheme support.
@JohnTitor
Copy link
Member

@bors rollup=always

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 17, 2020
Updating str.chars docs to mention crates.io.

This might spare someone else a little time searching the stdlib for unicode/grapheme support.
bors added a commit that referenced this pull request Feb 17, 2020
Rollup of 6 pull requests

Successful merges:

 - #68495 (Updating str.chars docs to mention crates.io.)
 - #68701 (Improve #Safety of various methods in core::ptr)
 - #69158 (Don't print block exit state in dataflow graphviz if unchanged)
 - #69179 (Rename `FunctionRetTy` to `FnRetTy`)
 - #69186 ([tiny] parser: `macro_rules` is a weak keyword)
 - #69188 (Clean up E0309 explanation)

Failed merges:

r? @ghost
@bors bors merged commit ac19dff into rust-lang:master Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants