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

Improved docstrings for free::chain #634

Merged
merged 1 commit into from Aug 25, 2022

Conversation

JoelMon
Copy link
Contributor

@JoelMon JoelMon commented Jul 11, 2022

Improved the description of the function and added example.

src/free.rs Outdated
@@ -128,16 +128,25 @@ pub fn zip<I, J>(i: I, j: J) -> Zip<I::IntoIter, J::IntoIter>
i.into_iter().zip(j)
}

/// Create an iterator that first iterates `i` and then `j`.
/// `chain()` returns the result of combining the _first_
Copy link
Contributor

Choose a reason for hiding this comment

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

fix: As in #633 (comment), this is not just about https://doc.rust-lang.org/nightly/std/primitive.array.htmls, so shouldn't be written this way.

@JoelMon JoelMon marked this pull request as draft July 11, 2022 19:39
@JoelMon
Copy link
Contributor Author

JoelMon commented Jul 11, 2022

@scottmcm: std::iter::chain this means that this function was also stabilized in std, right?

Copy link
Member

@phimuemue phimuemue left a comment

Choose a reason for hiding this comment

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

Hi there, thanks for this - improvements to documentation is always good!

It would be nice if you could incorporate the suggestions (or refute them with good reason), and sqash your changes into a single commit.

src/free.rs Outdated Show resolved Hide resolved
src/free.rs Outdated Show resolved Hide resolved
src/free.rs Outdated Show resolved Hide resolved
@JoelMon JoelMon force-pushed the chain-docstring branch 2 times, most recently from 94326f3 to 860f239 Compare August 25, 2022 16:51
@JoelMon
Copy link
Contributor Author

JoelMon commented Aug 25, 2022

I think I finally got this git squashing, force pushing stuff down. It's about time. 😆

@JoelMon JoelMon marked this pull request as ready for review August 25, 2022 16:54
@JoelMon JoelMon requested review from phimuemue and scottmcm and removed request for phimuemue and scottmcm August 25, 2022 16:55
Copy link
Member

@phimuemue phimuemue left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. Could you have a second look there? With the suggested changes, I think we're good to go.

src/free.rs Outdated Show resolved Hide resolved
src/free.rs Outdated Show resolved Hide resolved
@JoelMon
Copy link
Contributor Author

JoelMon commented Aug 25, 2022

@phimuemue, would you be able to take a look at #633 as well?

@JoelMon JoelMon requested a review from phimuemue August 25, 2022 20:03
@phimuemue
Copy link
Member

Thanks a bunch!

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 25, 2022

Build succeeded:

@bors bors bot merged commit 5ae7bb5 into rust-itertools:master Aug 25, 2022
@JoelMon JoelMon deleted the chain-docstring branch August 25, 2022 20:09
@jswrenn jswrenn added this to the 0.10.4 milestone Jun 21, 2023
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.

None yet

4 participants