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

If the book is going to show off enum variant comparison with ==, it also needs to talk about PartialEq #21196

Closed
bstrie opened this issue Jan 15, 2015 · 5 comments

Comments

@bstrie
Copy link
Contributor

bstrie commented Jan 15, 2015

A cousin to #17967.

The relevant bit of the docs is here: http://doc.rust-lang.org/book/compound-data-types.html#enums

The problem with this is that it's showing off the ability to compare enum variants with ==, which, aside from being something that almost never happens in practice, continually confounds users who expect it to automatically work on enums that they define themselves. That this section of the guide has been a stumbling block for so long (#17675, #18155) should be throwing up red flags by now.

If nothing else, it needs to contain a footnote/link/code comment giving some hint as to PartialEq's existence. If that would be out-of-scope for this chapter, then the section needs to be revised to eliminate the usage of ==.

@bstrie bstrie added the A-docs label Jan 15, 2015
@G4MR
Copy link

G4MR commented Jan 15, 2015

Yeah when I was running through the book my flow usually goes along the lines of recreating the example in a similar manner, but not exactly copying the code so when I went to create my example to see the basic usage shown in the example:

fn main()
{
    enum AgeScaling {
        Kid,
        Teen,
        Adult
    }

    let cmp = |&: age: i32| -> AgeScaling {
        if age <= 12 {
            AgeScaling::Kid
        } else if age < 18 && age > 12 {
            AgeScaling::Teen
        } else {
            AgeScaling::Adult
        }
    };  

    let yourscale = cmp(17);
    if yourscale == AgeScaling::Adult {
        println!("You're allowed to watch adult videos");
    } else {
        println!("Go away");
    }
}

I would get binary operator == annot be applied to type AgeScaling, which is confusing because my example is almost exactly similar to the example in the book. This made me think everything I was doing wrong so I googled and found out that I needed to use [derive(PartialEq)](actually it shown deriving, but I remember reading deriving was deprecated about a week or so ago when I was messing around with rust and sdl2 so I changed it to derive) above my enum and everything was sorted.

My issue with this is there will be multiple "beginners" reading this guide, doing exactly what I did and coming across googling and noticing the book doesn't talk about this usage at all. In my opinion the book shouldn't use the build in Ordering enum because it won't have the exact results as someone doing what I did and also talk about the [derive(...)] stuff explaining you haven't seen this before, but we'll explain it more in a future part of the book.

@estsauver
Copy link
Contributor

The pattern that I've seen in docs around this that I really appreciate is when guides go to the effort of showing how we could build standard things (like the ordering enum), and then showing that they already exist in the standard library.

If there's agreement we should make this change, I can put together a PR changing this section of the book.

@mdinger
Copy link
Contributor

mdinger commented Jan 15, 2015

I put up PR #21207 to try to address this. Comments may be helpful.

bors added a commit that referenced this issue 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
@mdinger
Copy link
Contributor

mdinger commented Feb 18, 2015

The enum section has changed since this was filed. If this bug is still valid, can you revise the summary accordingly.

@steveklabnik
Copy link
Member

Yes, I think this is fine now.

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

No branches or pull requests

5 participants