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

tokio-process: change CommandExt to a fully asynchronous Command struct #1448

Merged
merged 1 commit into from
Aug 18, 2019

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Aug 15, 2019

#1371 suggested to rewrite CommandExt in a way similar to tokio-fs (wrap the std struct and expose only asynchronous functions). I started rewriting Command in such a way.

Documentation and doctests are not updated yet, I just transformed the minimum amount of code to make tests pass. Please let me know if it's OK and if I shall continue :)

Refs: #1371

@carllerche carllerche requested a review from ipetkov August 15, 2019 16:32
Copy link
Member

@ipetkov ipetkov left a comment

Choose a reason for hiding this comment

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

Awesome work @Kobzol, looks great!

Happy to merge this in once the CI is squared away.

Feel free to ping me if you need help wording anything around the docs (though I imagine we can largely adapt what the std docs say and that should be enough)

tokio-process/src/lib.rs Outdated Show resolved Hide resolved
tokio-process/src/lib.rs Outdated Show resolved Hide resolved
tokio-process/src/lib.rs Outdated Show resolved Hide resolved
tokio-process/src/lib.rs Outdated Show resolved Hide resolved
tokio-process/src/lib.rs Outdated Show resolved Hide resolved
tokio-process/src/lib.rs Outdated Show resolved Hide resolved
tokio-process/src/lib.rs Outdated Show resolved Hide resolved
tokio-process/src/lib.rs Outdated Show resolved Hide resolved
tokio-process/src/lib.rs Outdated Show resolved Hide resolved
tokio-process/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@ipetkov ipetkov left a comment

Choose a reason for hiding this comment

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

LGTM, left some suggestions for some minor typo fixes. Also appears there's a merge conflict so we should rebase

@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 17, 2019

I applied the suggestions, squashed the commits and rebased onto master.

@ipetkov
Copy link
Member

ipetkov commented Aug 17, 2019

Seems the CI is failing now, looks like the imports need a quick tweak?

@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 18, 2019

Thanks, I screwed up solving a merge conflict while rebasing onto master.

@ipetkov
Copy link
Member

ipetkov commented Aug 18, 2019

CI was failing due to missing the fix added in #1464

I figured out how to update the branch after rebasing again on the latest master, let's see if this fixes the CI

@ipetkov
Copy link
Member

ipetkov commented Aug 18, 2019

CI is green, going to merge this in, thanks again @Kobzol!

@ipetkov ipetkov merged commit a9585f0 into tokio-rs:master Aug 18, 2019
@ipetkov ipetkov mentioned this pull request Aug 18, 2019
5 tasks
@Kobzol Kobzol deleted the process branch August 18, 2019 17:14
@carllerche
Copy link
Member

Thanks @Kobzol and @ipetkov for getting this done 👍

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.

3 participants