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.

Sign up for GitHub

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

Jump to bottom

Cleanup PartialEq docs. #57357

Merged
merged 8 commits into from
Jan 18, 2019
Merged

Cleanup PartialEq docs. #57357

merged 8 commits into from
Jan 18, 2019

Conversation

frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Jan 5, 2019

  • Cleanup the impl PartialEq<BookFormat> for Book implementation
  • Implement impl PartialEq<Book> for BookFormat so it’s symmetric
  • Removes the last example since it appears to be redundant with the
    previous two examples.

@frewsxcv
Cleanup PartialEq docs.
96678df 
- Cleanup the `impl PartialEq<BookFormat> for Book` implementation
- Implement `impl PartialEq<Book> for BookFormat` so it’s symmetric
  - Fixes rust-lang#53844.
- Removes the last example since it appears to be redundant with the
  previous two examples.
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2019
@frewsxcv frewsxcv mentioned this pull request Jan 5, 2019
Closed
@frewsxcv
Copy link
Member Author

frewsxcv commented Jan 5, 2019

r? @rust-lang/docs

@Centril
Copy link
Contributor

Centril commented Jan 5, 2019 •
edited
Loading

A side-effect of the changes here is that there's now no example that uses match, which feels like an important teaching opportunity wrt. PartialEq.. Could we perhaps work that in in some other way?

Also @frewsxcv, note that you cannot actually assign a team so don't use r? for that; just cc the team instead.

@Centril
Copy link
Contributor

Centril commented Jan 5, 2019

r? @steveklabnik

@frewsxcv
Copy link
Member Author

frewsxcv commented Jan 5, 2019

A side-effect of the changes here is that there's now no example that uses match, which feels like an important teaching opportunity wrt. PartialEq.. Could we perhaps work that in in some other way?

In my opinion, this doesn't seem like the right place to teach people about language features like match, that seems better suited for TRPL or other introductory learning resources.

Also @frewsxcv, note that you cannot actually assign a team so don't use r? for that; just cc the team instead.

Oh I know, I'm just asking someone from the docs team for a review.

@Centril
Copy link
Contributor

Centril commented Jan 5, 2019

In my opinion, this doesn't seem like the right place to teach people about language features like match, that seems better suited for TRPL or other introductory learning resources.

OK, you might be right, seems like a judgement call for Steve to make. ;)

Oh I know, I'm just asking someone from the docs team for a review.

Right, but when you r? a team, rust-highfive just gets unhappy and the PR then has no assignee... =P

@Centril Centril mentioned this pull request Jan 5, 2019
Closed
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
QuietMisdreavus
QuietMisdreavus reviewed Jan 7, 2019
View reviewed changes
Copy link
Member

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

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

I like adding the symmetric PartialEq impls, but i'm wary about removing the example you took out.

src/libcore/cmp.rs Show resolved Hide resolved
src/libcore/cmp.rs Outdated Show resolved Hide resolved
QuietMisdreavus
QuietMisdreavus approved these changes Jan 16, 2019
View reviewed changes
Copy link
Member

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@QuietMisdreavus
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 16, 2019

📌 Commit 32b2834 has been approved by QuietMisdreavus

@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 Jan 16, 2019
Centril added a commit to Centril/rust that referenced this pull request Jan 17, 2019
@Centril
Rollup merge of rust-lang#57357 - frewsxcv:frewsxcv-partial-eq, r=Qui…
4ceaa3a 
…etMisdreavus

Cleanup PartialEq docs.

- Cleanup the `impl PartialEq<BookFormat> for Book` implementation
- Implement `impl PartialEq<Book> for BookFormat` so it’s symmetric
  - Fixes rust-lang#53844.
- Removes the last example since it appears to be redundant with the
  previous two examples.
@Centril Centril mentioned this pull request Jan 17, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jan 17, 2019
@Centril
Rollup merge of rust-lang#57357 - frewsxcv:frewsxcv-partial-eq, r=Qui…
a319b86 
…etMisdreavus

Cleanup PartialEq docs.

- Cleanup the `impl PartialEq<BookFormat> for Book` implementation
- Implement `impl PartialEq<Book> for BookFormat` so it’s symmetric
  - Fixes rust-lang#53844.
- Removes the last example since it appears to be redundant with the
  previous two examples.
@Centril Centril mentioned this pull request Jan 17, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jan 17, 2019
@Centril
Rollup merge of rust-lang#57357 - frewsxcv:frewsxcv-partial-eq, r=Qui…
2e4392e 
…etMisdreavus

Cleanup PartialEq docs.

- Cleanup the `impl PartialEq<BookFormat> for Book` implementation
- Implement `impl PartialEq<Book> for BookFormat` so it’s symmetric
  - Fixes rust-lang#53844.
- Removes the last example since it appears to be redundant with the
  previous two examples.
@Centril Centril mentioned this pull request Jan 17, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jan 18, 2019
@Centril
Rollup merge of rust-lang#57357 - frewsxcv:frewsxcv-partial-eq, r=Qui…
c3c5d66 
…etMisdreavus

Cleanup PartialEq docs.

- Cleanup the `impl PartialEq<BookFormat> for Book` implementation
- Implement `impl PartialEq<Book> for BookFormat` so it’s symmetric
  - Fixes rust-lang#53844.
- Removes the last example since it appears to be redundant with the
  previous two examples.
@Centril Centril mentioned this pull request Jan 18, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jan 18, 2019
@Centril
Rollup merge of rust-lang#57357 - frewsxcv:frewsxcv-partial-eq, r=Qui…
7946095 
…etMisdreavus

Cleanup PartialEq docs.

- Cleanup the `impl PartialEq<BookFormat> for Book` implementation
- Implement `impl PartialEq<Book> for BookFormat` so it’s symmetric
  - Fixes rust-lang#53844.
- Removes the last example since it appears to be redundant with the
  previous two examples.
@Centril Centril mentioned this pull request Jan 18, 2019
Closed
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 18, 2019
@GuillaumeGomez
Rollup merge of rust-lang#57357 - frewsxcv:frewsxcv-partial-eq, r=Qui…
a9ea654 
…etMisdreavus

Cleanup PartialEq docs.

- Cleanup the `impl PartialEq<BookFormat> for Book` implementation
- Implement `impl PartialEq<Book> for BookFormat` so it’s symmetric
  - Fixes rust-lang#53844.
- Removes the last example since it appears to be redundant with the
  previous two examples.
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jan 18, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jan 18, 2019
@Centril
Rollup merge of rust-lang#57357 - frewsxcv:frewsxcv-partial-eq, r=Qui…
bca4908 
…etMisdreavus

Cleanup PartialEq docs.

- Cleanup the `impl PartialEq<BookFormat> for Book` implementation
- Implement `impl PartialEq<Book> for BookFormat` so it’s symmetric
  - Fixes rust-lang#53844.
- Removes the last example since it appears to be redundant with the
  previous two examples.
@Centril Centril mentioned this pull request Jan 18, 2019
Merged
bors added a commit that referenced this pull request Jan 18, 2019
@bors
Auto merge of #57737 - Centril:rollup, r=Centril
c76f3c3 
Rollup of 10 pull requests

Successful merges:

 - #56594 (Remove confusing comment about ideally using `!` for `c_void`)
 - #57340 (Use correct tracking issue for c_variadic)
 - #57357 (Cleanup PartialEq docs.)
 - #57551 (resolve: Add a test for issue #57539)
 - #57636 (Fix sources sidebar not showing up)
 - #57646 (Fixes text becoming invisible when element targetted)
 - #57654 (Add some links in std::fs.)
 - #57683 (Document Unpin in std::prelude documentation)
 - #57685 (Enhance `Pin` impl applicability for `PartialEq` and `PartialOrd`.)
 - #57710 (Fix non-clickable urls)

Failed merges:

r? @ghost
@bors bors merged commit 32b2834 into rust-lang:master Jan 18, 2019
@frewsxcv frewsxcv deleted the frewsxcv-partial-eq branch January 19, 2019 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuietMisdreavus QuietMisdreavus QuietMisdreavus approved these changes

Assignees

@QuietMisdreavus QuietMisdreavus

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@frewsxcv @rust-highfive @Centril @QuietMisdreavus @bors @steveklabnik @Kimundi