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

Provide documentation for Default impls #23271

Closed
wants to merge 1 commit into from

Conversation

frewsxcv
Copy link
Member

For the most part, the Default impls are pretty obvious as far as what
we expect them to return, but I think it's better to explicitly state
what's going to be returned in the documentation instead of relying on
the user to make assumptions/look at the source.

@rust-highfive
Copy link
Contributor

r? @brson

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

@frewsxcv frewsxcv force-pushed the default-docs branch 2 times, most recently from 9f3cb03 to 0bf338f Compare March 11, 2015 04:15
@@ -51,5 +51,6 @@ impl<H> Clone for DefaultState<H> {
}

impl<H> Default for DefaultState<H> {
/// Creates a new `DefaultState` containing `marker::PhantomData`
Copy link
Member

Choose a reason for hiding this comment

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

PhantomData is an implementation detail of DefaultState. It shouldn't be documented.

@frewsxcv frewsxcv force-pushed the default-docs branch 2 times, most recently from 779dded to a18037e Compare March 11, 2015 13:10
For the most part, the `Default` impls are pretty obvious as far as what
we expect them to return, but I think it's better to explicitly state
what's going to be returned in the documentation instead of relying on
the user to make assumptions/look at the source.
@frewsxcv
Copy link
Member Author

@sfackler The latest force push look okay?

@steveklabnik
Copy link
Member

I think it's better to explicitly state
what's going to be returned in the documentation instead of relying on
the user to make assumptions/look at the source.

In general, as you mention, we only document impls when it does something different than you'd expect. Given this rule, I'm not sure that this patch works.

I will say that how Rustdoc should show trait stuff is very much under discussion, that's just the current way we handle things.

@frewsxcv
Copy link
Member Author

we only document impls when it does something different than you'd expect

@steveklabnik Just looking at the documentation, could you tell me what you'd 'expect' for the Default impl for HashSet? hint: it's not HashSet::new.

I don't see the harm in documenting what a function returns.

@steveklabnik
Copy link
Member

Like most containers, I would expect it to consider the defaults of what it's generic over, which is exactly what it does.

I don't see the harm in documenting what a function returns.

I think the broader issue here isn't that more documentation is worse, it's just that we're unsure how to best do this documentation. And in my pull request for Arc docs, I actually did implement Default, but that didn't pass review due to the rule.

It's not that I think documentation is bad, it's just that consistency is also important. Maybe we should have a discussion about this topic in general.

@steveklabnik
Copy link
Member

Since the current convention is not to do this, I'm going to give it a close. @frewsxcv , if you want to have a discussion, we could have one over in the RFCs repo about the pros and cons of changing the convention.

@frewsxcv frewsxcv deleted the default-docs branch April 21, 2015 13:27
@frewsxcv
Copy link
Member Author

changing the convention.

@steveklabnik I'm not opposed this being closed, but for clarification, what convention are we talking about? Documenting Default impl or documenting impl in general?

@steveklabnik
Copy link
Member

The current convention is:

When writing an impl, only add documentation if that implementation does something non-obvious.

An example is Clone for Arc<T>: it doesn't actually copy any data, it bumps a reference count. So it gets documented. But Clone for Vec<T> doesn't, as it's just a deep copy.

@matthew-piziak
Copy link
Contributor

Part of #36265.

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