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

Added generic example of std::ops::Add in doc comments #41612

Merged
merged 1 commit into from
May 14, 2017

Conversation

mandeep
Copy link
Contributor

@mandeep mandeep commented Apr 28, 2017

We discussed on IRC how the std::ops examples were potentially missing examples using generics. This PR adds an example to std::ops::Add that shows the use of a generic type T. I'm not sure this is ready for merge as I think the two examples now make the documentation a bit verbose, but I think it's a good starting point. I'd love to hear others thoughts on this. This is in relation to the last item in issue #29365.

@rust-highfive
Copy link
Collaborator

r? @brson

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

@frewsxcv frewsxcv added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Apr 30, 2017
@frewsxcv
Copy link
Member

frewsxcv commented May 1, 2017

It seems like this example is being added to teach people how generic types work, which seems like a topic better suited for the book. That said, I imagine that using generic types when implementing Add could be a common scenario, so I'm alright with having this added.

@rust-lang/docs anyone else have opinions?

@frewsxcv frewsxcv added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label May 1, 2017
@mandeep
Copy link
Contributor Author

mandeep commented May 1, 2017

@frewsxcv I agree, in fact the first edition of the book contains an example similar to the one in this PR. Rather than having to rummage through the book, I was thinking that something like this example should be able to be found in the API docs. What do you think?

@GuillaumeGomez
Copy link
Member

Well, for me it's fine. It's just the trait Add which gets more examples.

Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

Just little nits. Also, some comments in the code might help users.

/// assert_eq!(Point { x: 1, y: 0 } + Point { x: 2, y: 3 },
/// Point { x: 3, y: 3 });
/// }
///
Copy link
Member

Choose a reason for hiding this comment

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

Extra empty line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the empty lines and comment regarding Output=T. Let me know if you'd like to see anything else.

Copy link
Member

@GuillaumeGomez GuillaumeGomez May 8, 2017

Choose a reason for hiding this comment

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

We now have two extra lines. :p (remove 272 and 273)

/// Point { x: 3, y: 3 });
/// }
///
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

Should be followed by an empty line.

@mandeep
Copy link
Contributor Author

mandeep commented May 7, 2017

Thinking about this some more. Maybe this example is best left in the book instead of the std lib docs. Later on I imagine something like this could go into a cookbook as well.

@GuillaumeGomez
Copy link
Member

I think it's a good thing to have it in multiple places. Can you just squash your commits please? Once done, I'll r+.

@mandeep
Copy link
Contributor Author

mandeep commented May 8, 2017

Thanks!

/// ```
/// use std::ops::Add;
///
/// #[derive(Debug)]
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any usage of the debug in the following code. Did you forget to remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other examples derived Debug, so I followed the same path. Do you want me to remove Debug in this example and the other examples in std::Ops?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please (strange we kept them).

@steveklabnik
Copy link
Member

@GuillaumeGomez looks like this was updated

@GuillaumeGomez
Copy link
Member

@steveklabnik: I commented on my previous review.

@mandeep
Copy link
Contributor Author

mandeep commented May 11, 2017

Looks like Debug was needed on a couple of the tests? How should we move forward?

@GuillaumeGomez
Copy link
Member

Put it where needed. My goal wasn't to force you to remove the Debug implementations but to remove the useless ones. If both of them are needed, we keep them of course.

@mandeep
Copy link
Contributor Author

mandeep commented May 11, 2017

My fault, I should have checked to see if Debug was needed.

@GuillaumeGomez
Copy link
Member

Ok, all good. Please squash and then I merge.

Added blank lines around example

Added comment to Add example referencing the Output type

Removed whitespace from lines 272 and 273

Removed Debug derivation from Add examples

Added Debug derivation
@frewsxcv
Copy link
Member

@bors r=GuillaumeGomez,frewsxcv rollup

@bors
Copy link
Contributor

bors commented May 13, 2017

📌 Commit a2a9d19 has been approved by GuillaumeGomez,frewsxcv

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 14, 2017
…meGomez,frewsxcv

Added generic example of std::ops::Add in doc comments

We discussed on IRC how the std::ops examples were potentially missing examples using generics. This PR adds an example to std::ops::Add that shows the use of a generic type T. I'm not sure this is ready for merge as I think the two examples now make the documentation a bit verbose, but I think it's a good starting point. I'd love to hear others thoughts on this. This is in relation to the last item in issue rust-lang#29365.
bors added a commit that referenced this pull request May 14, 2017
Rollup of 7 pull requests

- Successful merges: #41612, #41826, #41939, #41946, #41950, #41975, #41979
- Failed merges:
@bors bors merged commit a2a9d19 into rust-lang:master May 14, 2017
bors added a commit that referenced this pull request Jul 24, 2017
Add generic example of std::ops::Sub in doc comments

This PR adds an example of using generics with std::ops::Sub and is a follow up of PR #41612 and is related to issue #29365. I also wanted to add examples to Mul and Div, but I think these two traits are already loaded with examples.
@apiraino apiraino removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants