Skip to content

Add another PartialEq example - #51760

Merged
bors merged 1 commit into
rust-lang:masterfrom
GuillaumeGomez:add-another-partialeq-example
Aug 31, 2018
Merged

Add another PartialEq example#51760
bors merged 1 commit into
rust-lang:masterfrom
GuillaumeGomez:add-another-partialeq-example

Conversation

@GuillaumeGomez

@GuillaumeGomezGuillaumeGomez commented Jun 24, 2018

Copy link
Copy Markdown
Member

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 24, 2018
Centril
Centril reviewed Jun 24, 2018
View reviewed changes
Comment threadsrc/libcore/cmp.rs Outdated

@CentrilCentrilJun 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Space between ## and How and it => PartialEq imo

@GuillaumeGomezGuillaumeGomezJun 24, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

That's not the issue. ;)

@rust-highfive

rust-highfive commented Jun 24, 2018

Copy link
Copy Markdown
Contributor

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[01:16:28] Doc-tests core
[01:16:41] [01:16:41] running 2093 tests
[01:16:55] ....................................................................................................
[01:17:09] ........................................F...........................................................
[01:17:47] .................................i..................................................................
[01:18:00] ....................................................................................................
[01:18:13] ....................................................................................................
[01:18:27] ....................................................................................................

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@GuillaumeGomez
GuillaumeGomezforce-pushed the add-another-partialeq-example branch from e27a8eb to e69fa24CompareJune 24, 2018 22:32
@rust-highfive

rust-highfive commented Jun 24, 2018

Copy link
Copy Markdown
Contributor

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:00:52] sha256:8c1f0882640438d1d5e0c0acc857fedd1a16de0d9ee943660aa40de5c9b49bad
[00:00:52] Attempting with retry: docker build --rm -t rust-ci -f /home/travis/build/rust-lang/rust/src/ci/docker/x86_64-gnu-llvm-3.9/Dockerfile /home/travis/build/rust-lang/rust/src/ci/docker
[00:00:52] Sending build context to Docker daemon 501.2kB
[00:00:52] Step 1/6 : FROM ubuntu:16.04
[00:00:52] Get https://registry-1.docker.io/v2/library/ubuntu/manifests/16.04: received unexpected HTTP status: 503 Service Unavailable
[00:00:53] Sending build context to Docker daemon 501.2kB
[00:00:53] Step 1/6 : FROM ubuntu:16.04
[00:00:53] Step 1/6 : FROM ubuntu:16.04
[00:00:53] Get https://registry-1.docker.io/v2/library/ubuntu/manifests/16.04: received unexpected HTTP status: 503 Service Unavailable
[00:00:55] Sending build context to Docker daemon 501.2kB
[00:00:55] Step 1/6 : FROM ubuntu:16.04
[00:00:55] Step 1/6 : FROM ubuntu:16.04
[00:00:56] Get https://registry-1.docker.io/v2/library/ubuntu/manifests/16.04: received unexpected HTTP status: 503 Service Unavailable
[00:00:59] Sending build context to Docker daemon 501.2kB
[00:00:59] Step 1/6 : FROM ubuntu:16.04
[00:00:59] Step 1/6 : FROM ubuntu:16.04
[00:00:59] Get https://registry-1.docker.io/v2/library/ubuntu/manifests/16.04: received unexpected HTTP status: 503 Service Unavailable
[00:01:03] Sending build context to Docker daemon 501.2kB
[00:01:03] Step 1/6 : FROM ubuntu:16.04
[00:01:03] Step 1/6 : FROM ubuntu:16.04
[00:01:03] Get https://registry-1.docker.io/v2/library/ubuntu/manifests/16.04: received unexpected HTTP status: 503 Service Unavailable
[00:01:03] The command has failed after 5 attempts.
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 1.
travis_time:start:12abd860
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:03b2475d:start=1529879746989628774,finish=1529879746996691113,duration=7062339
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0e6474eb
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:2e1bf67f
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@rust-highfive

rust-highfive commented Jun 25, 2018

Copy link
Copy Markdown
Contributor

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@GuillaumeGomez
GuillaumeGomezforce-pushed the add-another-partialeq-example branch from e69fa24 to 59782beCompareJune 25, 2018 11:48
@GuillaumeGomez

GuillaumeGomez commented Jun 28, 2018

Copy link
Copy Markdown
MemberAuthor

ping @steveklabnik

@stokhos

stokhos commented Jul 6, 2018

Copy link
Copy Markdown

Ping from triage! @steveklabnik @rust-lang/docs , we haven't heard from you for a while, will youh ave time to look into this PR?

@stokhos

stokhos commented Jul 21, 2018

Copy link
Copy Markdown

Ping from triage, @rust-lang/docs anyone would want to review this PR?

@stokhosstokhos added I-nominated A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 21, 2018
@GuillaumeGomez

GuillaumeGomez commented Jul 21, 2018

Copy link
Copy Markdown
MemberAuthor

cc @QuietMisdreavus

QuietMisdreavus
QuietMisdreavus reviewed Jul 22, 2018
View reviewed changes
Comment threadsrc/libcore/cmp.rs Outdated

@QuietMisdreavusQuietMisdreavusJul 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure i agree with using "pretty easy" here. It's a poor phrase to use in documentation. How about something like this:

The type you can compare with is controlled by PartialEq's type parameter. For example, let's tweak our previous code a bit:

Then you can follow up below the example with something like:

By changing impl PartialEq for Book to impl PartialEq<BookFormat> for Book, we've changed what type we can use on the right side of the == operator. This lets us use it in the assert! statements at the bottom.

It would also be cool if you added one more example, combining them to show off that the same type can == to different types. Maybe continue with:

You can even combine these implementations to compare against both Book and BookFormat, like this:

(code sample with both PartialEq impls and both sets of assertions)

@GuillaumeGomez
GuillaumeGomezforce-pushed the add-another-partialeq-example branch from 59782be to b3f1f65CompareJuly 22, 2018 22:26
@GuillaumeGomez

GuillaumeGomez commented Jul 22, 2018

Copy link
Copy Markdown
MemberAuthor

Took your sentences and added the new code! (which was a good idea!)

QuietMisdreavus
QuietMisdreavus reviewed Jul 22, 2018
View reviewed changes
Comment threadsrc/libcore/cmp.rs Outdated

@QuietMisdreavusQuietMisdreavusJul 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of "of course" either. How about:

You can also combine these implementations to let the == operator work with two different types:

@GuillaumeGomezGuillaumeGomezJul 23, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fine by me! 😉

@GuillaumeGomez
GuillaumeGomezforce-pushed the add-another-partialeq-example branch from b3f1f65 to f92b2bbCompareJuly 23, 2018 08:10
@GuillaumeGomez

GuillaumeGomez commented Jul 23, 2018

Copy link
Copy Markdown
MemberAuthor

Updated.

steveklabnik
steveklabnik previously requested changes Jul 23, 2018
View reviewed changes

@steveklabniksteveklabnik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for missing the first ping :(

Comment threadsrc/libcore/cmp.rs Outdated

@steveklabniksteveklabnikJul 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This reads as awkward to me; can we get one last tweak?

/// ## How can I compare two different types?

Comment threadsrc/libcore/cmp.rs Outdated

@steveklabniksteveklabnikJul 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this how rustfmt formats this code?

@GuillaumeGomezGuillaumeGomezJul 23, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I took the code above as is so I don't know?

Comment threadsrc/libcore/cmp.rs Outdated

@steveklabniksteveklabnikJul 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

does this code need the &s? I'd expect them to not these days.

@GuillaumeGomezGuillaumeGomezAug 21, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It makes the code more clear.

Comment threadsrc/libcore/cmp.rs Outdated

@steveklabniksteveklabnikJul 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the `s should be around the whole "impl PartialEq for book" here

@TimNNTimNN added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team labels Aug 7, 2018
@TimNN

TimNN commented Aug 7, 2018

Copy link
Copy Markdown
Contributor

Ping from triage @GuillaumeGomez: Some changes have been requested to your PR.

@GuillaumeGomez

GuillaumeGomez commented Aug 8, 2018

Copy link
Copy Markdown
MemberAuthor

Oh right! I'll update soon.

@GuillaumeGomez
GuillaumeGomezforce-pushed the add-another-partialeq-example branch from f92b2bb to 663d6cdCompareAugust 11, 2018 11:24
@GuillaumeGomez

GuillaumeGomez commented Aug 11, 2018

Copy link
Copy Markdown
MemberAuthor

Updated to @steveklabnik's comments.

@XAMPPRockyXAMPPRocky removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 21, 2018
@XAMPPRocky

XAMPPRocky commented Aug 21, 2018

Copy link
Copy Markdown
Contributor

Triage: @steveklabnik This PR requires your review.

@GuillaumeGomez

GuillaumeGomez commented Aug 21, 2018

Copy link
Copy Markdown
MemberAuthor

Seems like I failed my rebase. I'll fix the conflicts.

@GuillaumeGomez
GuillaumeGomezforce-pushed the add-another-partialeq-example branch from 663d6cd to 3995bffCompareAugust 21, 2018 21:24
@GuillaumeGomez

GuillaumeGomez commented Aug 21, 2018

Copy link
Copy Markdown
MemberAuthor

Updated.

@emilyalbini

emilyalbini commented Aug 27, 2018

Copy link
Copy Markdown
Member

Ping from triage! This PR needs a review, can @steveklabnik or someone else from @rust-lang/docs review this?

@QuietMisdreavus

QuietMisdreavus commented Aug 27, 2018

Copy link
Copy Markdown
Contributor

Looks good to me! Let's get this merged.

@bors r+ rollup

@bors

bors commented Aug 27, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit 3995bff has been approved by QuietMisdreavus

@borsbors 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 Aug 27, 2018
@kennytmkennytm mentioned this pull request Aug 28, 2018
Closed
kennytm added a commit to kennytm/rust that referenced this pull request Aug 28, 2018
@kennytm
Rollup merge ofrust-lang#51760- GuillaumeGomez:add-another-partiale…
5b74d67
…q-example, r=QuietMisdreavus
Add another PartialEq example
r? @steveklabnik
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Aug 28, 2018
@Mark-Simulacrum
Rollup merge ofrust-lang#51760- GuillaumeGomez:add-another-partiale…
621d656
…q-example, r=QuietMisdreavus
Add another PartialEq example
r? @steveklabnik
@Mark-SimulacrumMark-Simulacrum mentioned this pull request Aug 28, 2018
Closed
emilyalbini added a commit to emilyalbini/rust that referenced this pull request Aug 29, 2018
@emilyalbini
Rollup merge ofrust-lang#51760- GuillaumeGomez:add-another-partiale…
2deda32
…q-example, r=QuietMisdreavus
Add another PartialEq example
r? @steveklabnik
@emilyalbiniemilyalbini mentioned this pull request Aug 29, 2018
Closed
emilyalbini added a commit to emilyalbini/rust that referenced this pull request Aug 29, 2018
@emilyalbini
Rollup merge ofrust-lang#51760- GuillaumeGomez:add-another-partiale…
68d19de
…q-example, r=QuietMisdreavus
Add another PartialEq example
r? @steveklabnik
@emilyalbiniemilyalbini mentioned this pull request Aug 29, 2018
Closed
@RalfJung

RalfJung commented Aug 30, 2018•
edited
Loading

Copy link
Copy Markdown
Member

These examples to not follow the rules the documentation states for ==: I can now do b1 == BookFormat::Paperback but not BookFormat::Paperback == b1, breaking symmetry. That does not seem nice.

Cc @gnzlbg

emilyalbini added a commit to emilyalbini/rust that referenced this pull request Aug 30, 2018
@emilyalbini
Rollup merge ofrust-lang#51760- GuillaumeGomez:add-another-partiale…
b7e74a5
…q-example, r=QuietMisdreavus
Add another PartialEq example
r? @steveklabnik
@emilyalbiniemilyalbini mentioned this pull request Aug 30, 2018
Merged
bors added a commit that referenced this pull request Aug 31, 2018
@bors
Auto merge of#53832- pietroalbini:rollup, r=pietroalbini
1114ab6
Rollup of 20 pull requests
Successful merges:
- #51760 (Add another PartialEq example)
- #53113 (Add example for Cow)
- #53129 (remove `let x = baz` which was obscuring the real error)
- #53389 (document effect of join on memory ordering)
- #53472 (Use FxHash{Map,Set} instead of the default Hash{Map,Set} everywhere in rustc.)
- #53476 (Add partialeq implementation for TryFromIntError type)
- #53513 (Force-inline `shallow_resolve` at its hottest call site.)
- #53655 (set applicability)
- #53702 (Fix stabilisation version for macro_vis_matcher.)
- #53727 (Do not suggest dereferencing in macro)
- #53732 (save-analysis: Differentiate foreign functions and statics.)
- #53740 (add llvm-readobj to llvm-tools-preview)
- #53743 (fix a typo: taget_env -> target_env)
- #53747 (Rustdoc fixes)
- #53753 (expand keep-stage --help text)
- #53756 (Fix typo in comment)
- #53768 (move file-extension based .gitignore down to src/)
- #53785 (Fix a comment in src/libcore/slice/mod.rs)
- #53786 (Replace usages of 'bad_style' with 'nonstandard_style'.)
- #53806 (Fix UI issues on Implementations on Foreign types)
Failed merges:
r? @ghost
@bors
bors merged commit 3995bff into rust-lang:masterAug 31, 2018
@RalfJungRalfJung mentioned this pull request Aug 31, 2018
Closed
@GuillaumeGomez
GuillaumeGomez deleted the add-another-partialeq-example branch August 31, 2018 07:56
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers
@CentrilCentrilCentril left review comments
@QuietMisdreavusQuietMisdreavusQuietMisdreavus left review comments
@steveklabniksteveklabniksteveklabnik left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@steveklabniksteveklabnik

Labels

A-docsArea: Documentation for any part of the project, including the compiler, standard library, and toolsS-waiting-on-borsStatus: 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.

11 participants

@GuillaumeGomez@rust-highfive@stokhos@TimNN@XAMPPRocky@emilyalbini@QuietMisdreavus@bors@RalfJung@steveklabnik@Centril