Skip to content

add default docstrings for String and AtomicBool #36364

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
Closed
matthew-piziak wants to merge 1 commit into from
Closed

add default docstrings for String and AtomicBool #36364

matthew-piziak wants to merge 1 commit into from

Conversation

@matthew-piziak
Copy link
Contributor

@matthew-piziak matthew-piziak commented Sep 9, 2016 •
edited
Loading

Part of #36265.

These are all the instances of Default in this repository that are of the form impl Default for X. The default for i32, for example, is not of this form.

r? @GuillaumeGomez

@matthew-piziak
Copy link
Contributor Author

matthew-piziak commented Sep 9, 2016

r? @GuillaumeGomez

@rust-highfive rust-highfive assigned GuillaumeGomez and unassigned nrc Sep 9, 2016
@frewsxcv
Copy link
Contributor

frewsxcv commented Sep 9, 2016

Relevant pull request, not sure if it's relevant anymore though #23271 . I'm still personally in favor of being explicit about what the default implementation does.

@matthew-piziak
Copy link
Contributor Author

matthew-piziak commented Sep 9, 2016

@frewsxcv Oops, didn't see that, thank you. Also, the reason my grep was so sparse was because I didn't catch type-parameterized impls. I'm inclined to close this PR. +1?

@frewsxcv
Copy link
Contributor

frewsxcv commented Sep 9, 2016

I think this can stay open. #23271 didn't merge, and I don't have any intention to reopen it.

@matthew-piziak
Copy link
Contributor Author

matthew-piziak commented Sep 9, 2016

@frewsxcv Okay, sounds good. I would have done this differently had I known what I do now, but it looks like these changes still constitute an improvement.

GuillaumeGomez
GuillaumeGomez reviewed Sep 9, 2016
View reviewed changes
src/libsyntax/ast.rs Outdated
Copy link
Member

@GuillaumeGomez GuillaumeGomez Sep 9, 2016

Choose a reason for hiding this comment

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

I don't think this is useful to write an explanation for an inner type.

@matthew-piziak matthew-piziak force-pushed the default-docs branch 2 times, most recently from de1550d to cec3335 Compare September 12, 2016 15:24
@matthew-piziak matthew-piziak changed the title add default docstrings for String, AtomicBool, and Generics add default docstrings for String and AtomicBool Sep 12, 2016
@matthew-piziak
add default docstrings for String and AtomicBool
a390564 
Part of rust-lang#36265.

These are all the instances of `Default` in this repository that are of the form `impl Default for X`. The default for `i32`, for example, is not of this form.

@GuillaumeGomez
@bors
Copy link
Collaborator

bors commented Sep 14, 2016

☔ The latest upstream changes (presumably #36472) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Oct 2, 2016

It seems it has already been done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@GuillaumeGomez GuillaumeGomez

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@matthew-piziak @frewsxcv @bors @GuillaumeGomez @nrc