Write a bunch of docs for char #28852

steveklabnik · 2015-10-05T22:09:41Z

Mostly adding examples, and reformatting for consistency.

rust-highfive · 2015-10-05T22:09:54Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

Gankra · 2015-10-05T22:10:38Z

cc @SimonSapin

Manishearth · 2015-10-05T22:24:24Z

src/librustc_unicode/char.rs

+    /// * `a-z`
+    /// * `A-Z`
+    ///
+    /// For a more comprehensive understanding of 'digit', see [``][is_numeric].


link doesn't have a body

Manishearth · 2015-10-05T22:36:35Z

src/librustc_unicode/char.rs

+    ///
+    /// assert_eq!(c.to_uppercase().next(), Some('C'));
+    ///
+    /// // 日本語 does not have case


The comment appears as three glyphs for me, but the character is one. Intentional?

Similar stuff happens in other Han/Kanji examples above.

Yes, as the three glyphs are how you write "Japanese" and "Chinese". I thought that it would be good to write them in their own scripts, given that we're in a crate based around Unicode. Of course, maybe that means I should be using "Türkçe" too, come to think of it.

Oh! Probably should mention the script name, not language name (Japanese, for one, has 3+ scripts), and romanize it too because in this case it looks like it's supposed to be the same as the character below.

Since case is a property of a language, and not neccesarily a script, as you mention, I was using the language name, but yeah, maybe that's just too confusing.

One difficulty is that 漢字 (the name of the writing system / script) is romanized differently depending on which language’s pronunciation you use. So maybe like this, to avoid picking sides?

/// // 漢字 (Kanji / Hanji) does not have case

Since case is a property of a language, and not necessarily a script

Is it? Is there any script that has case in one language but not in another language?

One other thing is that Japanese' Unicode representation includes special latin scripts which do have upper and lower case forms.

Manishearth · 2015-10-05T22:36:57Z

r=me with nits, though SImon should probably have a look first.

SimonSapin · 2015-10-05T22:53:47Z

src/librustc_unicode/char.rs

+    /// * Single quote is escaped as `\'`.
+    /// * Double quote is escaped as `\"`.
+    /// * Backslash is escaped as `\\`.
+    /// * Any character in the range `0x20` .. `0x7e` inclusive is not escaped.


Maybe mention that this is the range of "printable" characters in ASCII.

SimonSapin · 2015-10-05T23:34:49Z

All done reviewing. Thanks for the polish, Steve! This kind of work makes the difference between good and great documentation.

steveklabnik · 2015-10-06T00:05:53Z

@SimonSapin 😄

I've done everything except @Manishearth's 'radix' bit, so please feel free to re-review those fixes to make sure they work.

steveklabnik · 2015-10-06T02:41:56Z

Hmm, can't tell why the build failed here, is it just buried in the output somwhere?

SimonSapin · 2015-10-06T07:41:06Z

👍

Manishearth · 2015-10-06T07:45:51Z

src/librustc_unicode/char.rs

+    /// let c = 'Δ';
+    /// assert!(!c.is_lowercase());
+    ///
+    /// // The various Chinese languages do not have case, and so:


s/language/script

Hmmm this is an interesting question, I guess it is a property of a script and not a language

Manishearth · 2015-10-06T07:48:58Z

nitpick: case is a property of the script, not just the language. For example, capitalization if followed in romaji (Romanized Japanese). Otherwise lgtm

steveklabnik · 2015-10-06T12:45:32Z

still have to do the radix bit, but fixed everything else. I wonder if the tests will pass this time, I verified that they work locally, and still can't see where the error is on travis.

apasel422 · 2015-10-06T12:48:07Z

src/librustc_unicode/char.rs

@@ -195,21 +253,29 @@ impl char {
    #[inline]
    pub fn escape_unicode(self) -> EscapeUnicode { C::escape_unicode(self) }

-    /// Returns an iterator that yields the 'default' ASCII and
-    /// C++11-like literal escape of a character, as `char`s.
+    /// Returns an iterator of that yields the literal escape code of a `char`.


Should this just be "iterator that"?

apasel422 · 2015-10-06T12:49:03Z

src/librustc_unicode/char.rs

@@ -234,29 +300,69 @@ impl char {
    #[inline]
    pub fn escape_default(self) -> EscapeDefault { C::escape_default(self) }

-    /// Returns the number of bytes this character would need if encoded in
-    /// UTF-8.
+    /// Returns the number of bytes the character this `char` would need if encoded in UTF-8.


Should this just be "bytes this char would need"?

no, because the encoding really matters, it's not a well-formed statement otherwise.

Sorry, I meant that the full sentence seems like it should be:

Returns the number of bytes this `char` would need if encoded in UTF-8.

alexcrichton · 2015-10-06T15:36:33Z

src/librustc_unicode/char.rs

+    /// let result = thread::spawn(|| {
+    ///   let d = '1';
+    ///
+    ///   d.is_digit(37);


The indentation here looks like 2 spaces instead of 4, also perhaps this could have something like // this panics and the test could be annotated with should_panic?

This test does not actually panic though, as we spin up a thread and check the error.

(the spacing is my bad though)

True but it seems unidiomatic to spawn a thread in an example just to show that it fails? It seems like it may be distracting in terms of reading...

Oh, I see, change to that. I thought this was the preferred way to do it, but I guess I can change it if you'd prefer. We should have a good way to visually indicate this in rustdoc too...

steveklabnik · 2015-10-06T19:26:07Z

So both

$  make check-stage2-doc-crate-rustc_unicode
$  make check-stage2-doc-crate-std -j4

pass locally for me. So unsure about travis :/ @alexcrichton said he thinks it's due to the literal null in the source, but then not sure why it works for me.

alexcrichton · 2015-10-06T20:27:02Z

Ah sorry, right, it was make doc/std/index.html that was causing problems on travis

steveklabnik · 2015-10-06T21:06:22Z

The error message is amazing

rustdoc: doc/std/index.html
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: NulError(72, [60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 114, 117, 115, 116, 116, 101, 115, 116, 39, 62, 102, 110, 32, 109, 97, 105, 110, 40, 41, 32, 123, 10, 32, 32, 32, 32, 47, 47, 32, 78, 85, 76, 76, 44, 32, 85, 43, 48, 48, 48, 48, 10, 32, 32, 32, 32, 108, 101, 116, 32, 99, 32, 61, 32, 38, 35, 51, 57, 59, 0, 38, 35, 51, 57, 59, 59, 10, 32, 32, 32, 32, 97, 115, 115, 101, 114, 116, 33, 40, 99, 46, 105, 115, 95, 99, 111, 110, 116, 114, 111, 108, 40, 41, 41, 59, 10, 32, 32, 32, 32, 10, 32, 32, 32, 32, 108, 101, 116, 32, 99, 32, 61, 32, 38, 35, 51, 57, 59, 113, 38, 35, 51, 57, 59, 59, 10, 32, 32, 32, 32, 97, 115, 115, 101, 114, 116, 33, 40, 33, 99, 46, 105, 115, 95, 99, 111, 110, 116, 114, 111, 108, 40, 41, 41, 59, 10, 32, 32, 32, 32, 10, 125, 60, 47, 115, 112, 97, 110, 62, 60, 112, 114, 101, 32, 99, 108, 97, 115, 115, 61, 39, 114, 117, 115, 116, 32, 114, 117, 115, 116, 45, 101, 120, 97, 109, 112, 108, 101, 45, 114, 101, 110, 100, 101, 114, 101, 100, 39, 62, 10, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 99, 111, 109, 109, 101, 110, 116, 39, 62, 47, 47, 32, 78, 85, 76, 76, 44, 32, 85, 43, 48, 48, 48, 48, 60, 47, 115, 112, 97, 110, 62, 10, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 107, 119, 39, 62, 108, 101, 116, 60, 47, 115, 112, 97, 110, 62, 32, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 105, 100, 101, 110, 116, 39, 62, 99, 60, 47, 115, 112, 97, 110, 62, 32, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 111, 112, 39, 62, 61, 60, 47, 115, 112, 97, 110, 62, 32, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 115, 116, 114, 105, 110, 103, 39, 62, 38, 35, 51, 57, 59, 0, 38, 35, 51, 57, 59, 60, 47, 115, 112, 97, 110, 62, 59, 10, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 109, 97, 99, 114, 111, 39, 62, 97, 115, 115, 101, 114, 116, 60, 47, 115, 112, 97, 110, 62, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 109, 97, 99, 114, 111, 39, 62, 33, 60, 47, 115, 112, 97, 110, 62, 40, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 105, 100, 101, 110, 116, 39, 62, 99, 60, 47, 115, 112, 97, 110, 62, 46, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 105, 100, 101, 110, 116, 39, 62, 105, 115, 95, 99, 111, 110, 116, 114, 111, 108, 60, 47, 115, 112, 97, 110, 62, 40, 41, 41, 59, 10, 10, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 107, 119, 39, 62, 108, 101, 116, 60, 47, 115, 112, 97, 110, 62, 32, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 105, 100, 101, 110, 116, 39, 62, 99, 60, 47, 115, 112, 97, 110, 62, 32, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 111, 112, 39, 62, 61, 60, 47, 115, 112, 97, 110, 62, 32, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 115, 116, 114, 105, 110, 103, 39, 62, 38, 35, 51, 57, 59, 113, 38, 35, 51, 57, 59, 60, 47, 115, 112, 97, 110, 62, 59, 10, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 109, 97, 99, 114, 111, 39, 62, 97, 115, 115, 101, 114, 116, 60, 47, 115, 112, 97, 110, 62, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 109, 97, 99, 114, 111, 39, 62, 33, 60, 47, 115, 112, 97, 110, 62, 40, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 111, 112, 39, 62, 33, 60, 47, 115, 112, 97, 110, 62, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 105, 100, 101, 110, 116, 39, 62, 99, 60, 47, 115, 112, 97, 110, 62, 46, 60, 115, 112, 97, 110, 32, 99, 108, 97, 115, 115, 61, 39, 105, 100, 101, 110, 116, 39, 62, 105, 115, 95, 99, 111, 110, 116, 114, 111, 108, 60, 47, 115, 112, 97, 110, 62, 40, 41, 41, 59, 60, 47, 112, 114, 101, 62, 10])', src/libcore/result.rs:736

Looks like I'll have to remove the NULL for now, and file a rustdoc bug

Mostly adding examples, and reformatting for consistency.

steveklabnik · 2015-10-07T14:42:14Z

okay, travis passed. Everyone happy now? :)

SimonSapin · 2015-10-07T16:13:15Z

Looks good to me.

alexcrichton · 2015-10-07T17:22:35Z

@bors: r+ 51097fc

bors · 2015-10-07T19:40:05Z

⌛ Testing commit 51097fc with merge babe953...

Mostly adding examples, and reformatting for consistency.

bors · 2015-10-07T21:25:41Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned nikomatsakis Oct 5, 2015

Manishearth reviewed Oct 5, 2015
View reviewed changes

steveklabnik force-pushed the doc_char branch 2 times, most recently from 7fb7a95 to d1a917a Compare October 5, 2015 22:32

Manishearth reviewed Oct 5, 2015
View reviewed changes

SimonSapin reviewed Oct 5, 2015
View reviewed changes

steveklabnik force-pushed the doc_char branch from d1a917a to 63444e7 Compare October 6, 2015 00:05

Manishearth reviewed Oct 6, 2015
View reviewed changes

steveklabnik force-pushed the doc_char branch 2 times, most recently from f7a32e5 to 78a71fa Compare October 6, 2015 12:45

apasel422 reviewed Oct 6, 2015
View reviewed changes

steveklabnik force-pushed the doc_char branch 3 times, most recently from 6858e83 to 16b2250 Compare October 6, 2015 13:46

alexcrichton reviewed Oct 6, 2015
View reviewed changes

steveklabnik force-pushed the doc_char branch from 16b2250 to 441e141 Compare October 6, 2015 17:58

steveklabnik force-pushed the doc_char branch from 441e141 to 51097fc Compare October 6, 2015 21:58

Write a bunch of docs for char

51097fc

Mostly adding examples, and reformatting for consistency.

bors added a commit that referenced this pull request Oct 7, 2015

Auto merge of #28852 - steveklabnik:doc_char, r=alexcrichton

babe953

Mostly adding examples, and reformatting for consistency.

bors merged commit 51097fc into rust-lang:master Oct 7, 2015

Write a bunch of docs for char #28852

Write a bunch of docs for char #28852

Uh oh!

Conversation

steveklabnik commented Oct 5, 2015

Uh oh!

rust-highfive commented Oct 5, 2015

Uh oh!

Gankra commented Oct 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Oct 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimonSapin commented Oct 5, 2015

Uh oh!

steveklabnik commented Oct 6, 2015

Uh oh!

steveklabnik commented Oct 6, 2015

Uh oh!

SimonSapin commented Oct 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Oct 6, 2015

Uh oh!

steveklabnik commented Oct 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveklabnik commented Oct 6, 2015

Uh oh!

alexcrichton commented Oct 6, 2015

Uh oh!

steveklabnik commented Oct 6, 2015

Uh oh!

steveklabnik commented Oct 7, 2015

Uh oh!

SimonSapin commented Oct 7, 2015

Uh oh!

alexcrichton commented Oct 7, 2015

Uh oh!

bors commented Oct 7, 2015

Uh oh!