-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Many From
implementations are undocumented #51430
#51738
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @bluss (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
The job 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 |
I appreciate the effort to document these conversions! I think that right now the way rustdoc works, it's kind of hard to actually see the documentation for these. The docs for I think that adding docs on the impls themselves is a good thing, although maybe it might be better for some of these to put them elsewhere so they're also seen. For example, the impl for Actually figuring out how to make these docs easy to find is a difficult problem, so, I think that modifying what you've got so far is definitely fine for now. Congrats on your first pull request. :) |
Hi @clarcharr, I think #51430 is the better place for that kind of feedback. Rustdoc drowning From implementations is an issue but IMHO not in scope for this. |
☔ The latest upstream changes (presumably #51944) made this pull request unmergeable. Please resolve the merge conflicts. |
Thanks for this PR @ormrepo! The build is failing, and there are conflicts that needs to be solved. Do you have time to fix those? |
Hi Pietro,
Sorry to hear about that. I submitted this a few weeks ago and I was hoping
that @skade <https://github.com/skade> would have responded to this by now.
Unfortunately he has disappeared for some unknown reason.
As this was my first PR I was hoping that my mentor @skade
<https://github.com/skade> would have assisted with this he may be away on
holiday or something.
Sorry about this.
Sehinde
On 9 July 2018 at 11:55:40, Pietro Albini ([email protected]) wrote:
Thanks for this PR @ormrepo <https://github.com/ormrepo>! The build is
failing, and there are conflicts that needs to be solved. Do you have time
to fix those?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#51738 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AF64cERaZfx29CDnkUhhBDuocYehA8g8ks5uEzasgaJpZM4U03Iv>
.
|
@ormrepo don't worry, I'm just checking in to make sure PRs are not abandoned |
Ping from triage @skade @rust-lang/docs, this PR needs your review. |
Ping from triage! This PR needs a review, can @skade or someone else from @rust-lang/docs review this? |
Won't the conversions also show up on the types with the |
@@ -414,38 +414,74 @@ impl<T: ?Sized + Hasher> Hasher for Box<T> { | |||
} | |||
} | |||
|
|||
/// First From |
There was a problem hiding this comment.
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 this type of comment in general, but especially not as a doc comment. This won't make much sense to appear in the generated documentation.
In code, these sorts of comments are possibly useful for navigation, but they are subject to bitrot and I think that they are consequently not that valuable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
/// Converts a T into a Box. | ||
/// This conversion does allocate memory. | ||
/// This conversion is not free. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant blank line.
/// Copies a T in to a box. | ||
/// This conversion copies the data. | ||
/// This conversion does allocate memory. | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extraneous blank line.
fn from(slice: &'a [T]) -> Box<[T]> { | ||
let mut boxed = unsafe { RawVec::with_capacity(slice.len()).into_box() }; | ||
boxed.copy_from_slice(slice); | ||
boxed | ||
} | ||
/// The result is allocated on the heap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doc comment doesn't apply to anything. There are a number of these and they should be incorporated into the actual docs or removed. If you think there is value to leaving explanatory comments in the code, I think that they should be a separate PR so that libs team can evaluate them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
Allercah
Your comment is not helpful or useful. There are no clear examples to
demonstrate what you are saying.
This is typical developer speak please put clothes on your argument…..
On 31 July 2018 at 06:55:32, Alexis Hunt ([email protected]) wrote:
*@alercah* requested changes on this pull request.
------------------------------
In src/liballoc/boxed.rs
<#51738 (comment)>:
@@ -414,38 +414,74 @@ impl<T: ?Sized + Hasher> Hasher for Box<T> {
}
}
+/// First From
I'm not a fan of this type of comment in general, but especially not as a
doc comment. This won't make much sense to appear in the generated
documentation.
In code, these sorts of comments are possibly useful for navigation, but
they are subject to bitrot and I think that they are consequently not that
valuable.
------------------------------
In src/liballoc/boxed.rs
<#51738 (comment)>:
#[stable(feature = "from_for_ptrs", since = "1.6.0")]
impl<T> From<T> for Box<T> {
+ /// Converts a T into a Box.
+ /// This conversion does allocate memory.
+ /// This conversion is not free.
+
Redundant blank line.
------------------------------
In src/liballoc/boxed.rs
<#51738 (comment)>:
#[stable(feature = "box_from_slice", since = "1.17.0")]
impl<'a, T: Copy> From<&'a [T]> for Box<[T]> {
+ /// Copies a T in to a box.
+ /// This conversion copies the data.
+ /// This conversion does allocate memory.
+ ///
Extraneous blank line.
------------------------------
In src/liballoc/boxed.rs
<#51738 (comment)>:
fn from(slice: &'a [T]) -> Box<[T]> {
let mut boxed = unsafe {
RawVec::with_capacity(slice.len()).into_box() };
boxed.copy_from_slice(slice);
boxed
}
+ /// The result is allocated on the heap
This doc comment doesn't apply to anything. There are a number of these and
they should be incorporated into the actual docs or removed. If you think
there is value to leaving explanatory comments in the code, I think that
they should be a separate PR so that libs team can evaluate them.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#51738 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AF64cNZm7oyTxAnewp9gEXGVF4TYeH0cks5uL_FTgaJpZM4U03Iv>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Steve for the clarification. And I agree with you. I don't have permission to approve these changes. I am a newbie here. Can someone please approve these changes and close down the PR request ? Or is this PR request owned by @skade I haven't heard from him in nearly 6 weeks.......
I'm assigning @steveklabnik as the reviewer here. I believe the next step is to rebase and fix the comments left by @alercah and @steveklabnik. |
Ping from triage, @ormrepo. It looks like some changes have been requested to your PR. |
Hi Triage or Tim,
I agree with @steveklabnik
I don’t have write access to approve this PR and I have been waiting on
@skade to approve the PR.
I have tried on multiple occasions to contact @skade and I have been
unsuccessful.
I am new to PR’s what is the process for assigning a PR if that person
refuses to respond ?
Regards
Sehinde
On 14 August 2018 at 17:32:49, Tim Neumann ([email protected]) wrote:
Ping from triage, @ormrepo <https://github.com/ormrepo>. It looks like some
changes have been requested to your PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#51738 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AF64cMFadFnJng1U7eX85U-DMX6CVHRSks5uQvuwgaJpZM4U03Iv>
.
|
Thank you for contributing to the Rust project, @ormrepo. Please note that this Pull Request cannot be approved right now. Reviewers have requested some changes, which you need to make before approval can be granted. (Note that To make the requested changes, you'll have to go to your local checkout of the Edit: I forgot to mention: after making you changes, you'll also need to rebase you branch. How do I rebase?
Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how |
Hi Tim,
I have made the changes on my branch and I am having some problems pushing
them up. I don't have an upstream branch specified, this means that I can’t
pull down the latest updates in order to rebase. I am at a loss right now
on how to move on. Do you have any ideas ?
Regards
Sehinde
On 15 August 2018 at 10:13:17, Tim Neumann ([email protected]) wrote:
Thank you for contributing to the Rust project, @ormrepo
<https://github.com/ormrepo>. Please note that this Pull Request cannot be
approved right now. Reviewers have requested some changes, which you need
to make before approval can be granted. (Note that @skade does not
necessarily have to be involved in the process if they are unreachable at
the moment. Others, like @steveklabnik, can also approve this PR once the
requested changes have been made).
------------------------------
To make the requested changes, you'll have to go to your local checkout of
the boxrs_edits branch, make the requested changes, and then finally commit
and push them to GitHub.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#51738 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AF64cCDs7pamMpIA_syKA0wJEJvyfI_Bks5uQ-YtgaJpZM4U03Iv>
.
|
You can see your defined remotes by running If you don't have one, you can use one of these commands to set one up: git remote add upstream [email protected]:ormrepo/rust.git
# Or, if you don't have ssh set up
git remote add upstream https://github.com/ormrepo/rust.git |
Ping from triage @ormrepo! It's been a while since we heard from you, will you have time to work on this again? |
Ping from triage @ormrepo: We haven't heard from you in a while so I'm closing this PR as per our triage guidelines. Thank you for your contribution and please re-open this PR (or a new one) when you are able to work on this PR again. |
Hi, @skade This is my first pull request (Please be gentle with me....). I have followed your instructions, there could be a few mistakes. Let me know what they are and I will improve upon them.