Skip to content

Conversation

@mdinger
Copy link
Contributor

@mdinger mdinger commented Feb 2, 2015

Second try to address #21196 . A lot that was removed at the end basically seemed repetitive showing simple variations on the same type. It seems more effective to just show more variants at the beginning instead.

If you want to pack values into an example, better to use i32 or some digit than String because you don't need the to_string() method.

I didn't mention derive because:

  • I can't explain it (only use it)
  • I don't have a link to a good description (maybe rustbyexample but you probably want links internal)
  • Giving more detail especially stating that == won't work and why should help quite a bit

I didn't make test or check links but I will if this will be merged.

@steveklabnik

@rust-highfive
Copy link
Contributor

r? @pcwalton

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

@mdinger
Copy link
Contributor Author

mdinger commented Feb 2, 2015

r? @steveklabnik

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need the comma here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@steveklabnik
Copy link
Contributor

Thanks for this @mdinger ! I think that conceptually, I like this patch. We need to do some wordsmithing, but I like the general approach.

@mdinger
Copy link
Contributor Author

mdinger commented Feb 2, 2015

Oh good. Thanks for the quick look over. Long lead times get frustrating.

Actually, I think I may be able to work in #[derive(...)] but I gotta think about it. The way it's usually used, it tends to feel like black magic. Flip the switch and everything works but we don't explain how.

@steveklabnik
Copy link
Contributor

Yeah, I'm at an airport right now, about to fly from Europe to Australia, so apologies if there ends up being a longer time for reviewing. For the benefit of other committers, since this is mostly grammar nitpicking, I'm comfortable r+ing this, since I'm going to end up doing a full editing pass later, and it is just nitpicks.

Actually, I think I may be able to work in #[derive(...)]

I don't think that's a good idea: we haven't covered any of the relevant information yet.

@mdinger
Copy link
Contributor Author

mdinger commented Feb 2, 2015

Okay. I'll do make check and check the links in the morning.

I do tend to agree but this still leaves it open to the reader deciding "I want to define Ordering myself". But it'll just have to be dealt with, at least for now.

By the way, you guys typically have amazing response times. Especially when it's just a few minutes and since you deal with 100's of emails a day.

@steveklabnik
Copy link
Contributor

By the way, you guys typically have amazing response times. Especially when it's just a few minutes and since you deal with 100's of emails a day.

Thanks. 😄

@mdinger
Copy link
Contributor Author

mdinger commented Feb 2, 2015

make check passed and the links work. I don't know what this formatting note means but it didn't error. The formatting section looks different now. Weird.

check: formatting
...
249 error codes
highest error code: E0315

@steveklabnik r?

@steveklabnik
Copy link
Contributor

@bors: r+ a9583d6

@bors
Copy link
Collaborator

bors commented Feb 6, 2015

⌛ Testing commit a9583d6 with merge 581b4f1...

@bors
Copy link
Collaborator

bors commented Feb 6, 2015

💔 Test failed - auto-mac-64-nopt-t

@alexcrichton
Copy link
Member

@bors: retry

bors added a commit that referenced this pull request Feb 6, 2015
Second try to address #21196 . A lot that was removed at the end basically seemed repetitive showing simple variations on the same type. It seems more effective to just show more variants at the beginning instead.

If you want to pack values into an example, better to use `i32` or some digit than `String` because you don't need the `to_string()` method.

I didn't mention `derive` because:
* I can't explain it (only use it)
* I don't have a link to a good description (maybe rustbyexample but you probably want links internal)
* Giving more detail especially stating that `==` won't work and why should help quite a bit

I didn't `make test` or check links but I will if this will be merged.

@steveklabnik
@bors
Copy link
Collaborator

bors commented Feb 6, 2015

⌛ Testing commit a9583d6 with merge 7884eb8...

@bors
Copy link
Collaborator

bors commented Feb 6, 2015

@bors bors merged commit a9583d6 into rust-lang:master Feb 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants