Skip to content

Conversation

@kelebra
Copy link
Contributor

@kelebra kelebra commented Feb 20, 2018

This PR addresses issue #24519. Open questions so far:

  • Does this one deserve on its own class level implementation (like LazySource and friends)?
  • Should it be added to Cookbook (stream-cookbook.md)?

@akka-ci
Copy link

akka-ci commented Feb 20, 2018

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

@ktoso
Copy link
Contributor

ktoso commented Feb 20, 2018

OK TO TEST

@akka-ci akka-ci added the validating PR is currently being validated by Jenkins label Feb 20, 2018
Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Almost there, please add a test too


---------------------------------------------------------------

### actorPublisher
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be here anymore :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops, removed.

* Creates a `Source` from supplied future factory that is not called until downstream demand. When source gets
* materialized the materialized future is completed with the value from the factory. If downstream cancels or fails
* without any demand the create factory is never called and the materialized `Future` is failed.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a link to the normal one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, done.

* materialized the materialized future is completed with the value from the factory. If downstream cancels or fails
* without any demand the create factory is never called and the materialized `Future` is failed.
*/
def lazilyAsync[T](create: () CompletionStage[T]): Source[T, Future[NotUsed]] =
Copy link
Contributor

Choose a reason for hiding this comment

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

This should work on Scala.concurrent.Future :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, also done.

@akka-ci akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Feb 20, 2018
@akka-ci
Copy link

akka-ci commented Feb 20, 2018

Test PASSed.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Feb 21, 2018
@kelebra
Copy link
Contributor Author

kelebra commented Feb 21, 2018

Sure, was on it. Added unit tests similar to the ones from LazySourceSpec.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Feb 21, 2018
@akka-ci
Copy link

akka-ci commented Feb 21, 2018

Test PASSed.

@akka-ci akka-ci added the tested PR that was successfully built and tested by Jenkins label Feb 21, 2018
@akka-ci
Copy link

akka-ci commented Feb 21, 2018

Test PASSed.

@@ -0,0 +1,77 @@
package akka.stream.scaladsl
Copy link
Contributor

Choose a reason for hiding this comment

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

All files must have the header:

/**
 * Copyright (C) 2014-2018 Lightbend Inc. <https://www.lightbend.com>
 */

We'll soon automate that I hope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added but also see that you did that already, sorry for kind of a mess :(

Copy link
Contributor

@ktoso ktoso Feb 21, 2018

Choose a reason for hiding this comment

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

Oh, sorry -- I somehow expected you be in EU timezone so was taking my time prepping and going to merge this while EU is asleep :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CST actually :) but thanks for taking care of it, much appreciated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I could work on automating copyright check if you can supply issue for it

Copy link
Contributor

Choose a reason for hiding this comment

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

Cheers to CST from JST then :)

For the copyright we want to replicate what we have here: akka/akka-http#1738

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Feb 21, 2018
@akka-ci
Copy link

akka-ci commented Feb 21, 2018

Test PASSed.

@akka-ci akka-ci added the validating PR is currently being validated by Jenkins label Feb 21, 2018
@akka-ci akka-ci removed the tested PR that was successfully built and tested by Jenkins label Feb 21, 2018
@ktoso
Copy link
Contributor

ktoso commented Feb 21, 2018

Does this one deserve on its own class level implementation (like LazySource and friends)?

Nope, it's good as is.

Should it be added to Cookbook (stream-cookbook.md)?

Nope, it's enough to have it in the overview doc. Cookbook is for "this is how you can put together a number of things to achieve some thing"

@ktoso
Copy link
Contributor

ktoso commented Feb 21, 2018

LGTM, simple enough that we can merge once validated I think 👍

@akka-ci akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Feb 21, 2018
@akka-ci
Copy link

akka-ci commented Feb 21, 2018

Test PASSed.

@kelebra
Copy link
Contributor Author

kelebra commented Feb 21, 2018

I think all issues highlighted in PR were addressed. Is there anything else required for this PR to be merged?

@ktoso
Copy link
Contributor

ktoso commented Feb 22, 2018

Lookin good, thanks 👍

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

Labels

tested PR that was successfully built and tested by Jenkins

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants