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

bump futures and friends and use async/await where possible #140

Merged
merged 13 commits into from Jan 29, 2020

Conversation

@steveeJ
Copy link
Contributor

steveeJ commented Jan 14, 2020

This is based on #138 (thanks to @schrieveslaach!).

I'm happy to squash the new commits into the one's made by @schrieveslaach if we reach consensus on doing so.

tests/mock/tags.rs Outdated Show resolved Hide resolved
tests/mock/catalog.rs Outdated Show resolved Hide resolved
tests/mock/catalog.rs Outdated Show resolved Hide resolved
examples/image.rs Outdated Show resolved Hide resolved
examples/tags.rs Outdated Show resolved Hide resolved
src/errors.rs Show resolved Hide resolved
src/v2/auth.rs Outdated Show resolved Hide resolved
src/v2/auth.rs Outdated Show resolved Hide resolved
src/v2/catalog.rs Outdated Show resolved Hide resolved
src/v2/catalog.rs Outdated Show resolved Hide resolved
src/v2/mod.rs Show resolved Hide resolved
tests/mock/catalog.rs Outdated Show resolved Hide resolved
tests/mock/tags.rs Outdated Show resolved Hide resolved
@steveeJ steveeJ force-pushed the steveeJ-forks:pr/async-await-pr138continuation branch 2 times, most recently from 7d6d094 to ca75269 Jan 17, 2020
@steveeJ

This comment has been minimized.

Copy link
Contributor Author

steveeJ commented Jan 17, 2020

@lucab thanks a lot for the thorough review. I addressed all the comments. Please take another look.

@steveeJ steveeJ force-pushed the steveeJ-forks:pr/async-await-pr138continuation branch from c604467 to 20c6c65 Jan 17, 2020
@steveeJ steveeJ changed the title [WIP] bump futures and friends and use async/await where possible bump futures and friends and use async/await where possible Jan 17, 2020
@lucab

This comment has been minimized.

Copy link
Member

lucab commented Jan 21, 2020

No further remarks from my side. I'll wait for the squash+rebase at #138 (comment) to happen and then do the final stamp on this.

schrieveslaach and others added 13 commits Jan 10, 2020
The motivation is to reduce code complexity.

It removes the necessity of Box'ing the Stream, and moves the
responsibility of doing so if necessary to the consumer.
This feature will be used to gate tests which require credentials.
In addition to adding a feature-gate for the authenticated tests, this
makes the test fail if the authentication fails. This way, if the
feature `test-net-private` is enabled, the tests will consistently fail
if they can't run to completion.
* Instead of running the streams to completion, abort on first
  encountered error
* Output unexpected results
@steveeJ steveeJ force-pushed the steveeJ-forks:pr/async-await-pr138continuation branch from 20c6c65 to b6e332c Jan 24, 2020
pub fn get_catalog<'a, 'b: 'a>(
&'b self,
paginate: Option<u32>,
) -> impl Stream<Item = Result<String>> + 'a {

This comment has been minimized.

Copy link
@schrieveslaach
Copy link
Contributor

schrieveslaach left a comment

Looks good to me. 👍

The tests could be made simpler by using tokio::test but that should not block the PR.

@@ -70,29 +94,13 @@ fn test_quayio_insecure() {
assert_eq!(res, true);
}

#[cfg(feature = "test-net-private")]
#[test]

This comment has been minimized.

Copy link
@schrieveslaach

schrieveslaach Jan 28, 2020

Contributor

What about #[tokio::test]? Then you wouldn't have to create a Runtime in each test.

@steveeJ

This comment has been minimized.

Copy link
Contributor Author

steveeJ commented Jan 28, 2020

Thanks for the review @schrieveslaach!

The tests could be made simpler by using tokio::test but that should not block the PR.

I gave it a quick try. I would have had to give it more thought than I want to for the scope of this PR, when refactoring the tests which test the impl Stream functions in the mock tests. I'll be happy to do a follow-up PR with that refactor. I want to give you and @lucab another chance to express any strong feelings about blocking the PR on this, otherwise I think we're good to merge it.

@schrieveslaach

This comment has been minimized.

Copy link
Contributor

schrieveslaach commented Jan 29, 2020

A follow-up PR sounds good to me.

@lucab
lucab approved these changes Jan 29, 2020
@lucab

This comment has been minimized.

Copy link
Member

lucab commented Jan 29, 2020

LGTM, thanks both of you!

@steveeJ steveeJ merged commit 712f7da into camallo:master Jan 29, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@steveeJ steveeJ deleted the steveeJ-forks:pr/async-await-pr138continuation branch Jan 29, 2020
@lucab

This comment has been minimized.

Copy link
Member

lucab commented Jan 29, 2020

@schrieveslaach @steveeJ does any of you need this to be part of a release in the short term, or can we keep going and lazily tag one later on?

@schrieveslaach

This comment has been minimized.

Copy link
Contributor

schrieveslaach commented Jan 29, 2020

Would be nice if I could use a release(-candidate) in aixigo/PREvant#28 but you don't have to hurry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.