use LazyList instead of Stream in S3Stream

apache / pekko-connectors

Apache Pekko Connectors is a Reactive Enterprise Integration library for Java and Scala, based on Reactive Streams and Apache Pekko.

https://pekko.apache.org/

Apache License 2.0

59 stars 32 forks source link

use LazyList instead of Stream in S3Stream #71

Closed pjfanning closed 1 year ago

pjfanning commented 1 year ago

As part of reintroducing Scala 2.12 support, we reverted the Alpakka change to use LazyList and went back to the Scala Stream class.

we could modify the code so that Scala 2.12 code uses Stream and Scala 2.13+ uses LazyList (new in 2.13 but preferred)
we could copy in the code from scala-collection-compat that does this or we can write our own - if we use scala-collection-compat code, we will need to update our LICENSE and NOTICE
we could add a dependency in the s3 module to scala-collection-compat - avoids us having to update our LICENSE and NOTICE

pjfanning commented 1 year ago

@mdedetrich this issue is about possibly using LazyList again

mdedetrich commented 1 year ago

@mdedetrich this issue is about possibly using LazyList again

Ah I see

One thing I want to mention is that the usage of Stream/LazyList in the first place might be an oversight since Pekko provides its own way to generate a stream of values.

In other words we might be able to just use Source.repeat instead of Stream/LazyList.continuously. Ontop of this Stream/LazyList.continuously is only being used in tests and the only non test code that uses Stream/LazyList.continuously is in S3Stream functions that are deprecated.

pjfanning commented 1 year ago

@mdedetrich Fair enough. I'm not treating this as an issue with any urgency. It might be best to leave it open for further discussion but I don't intend to make any changes myself based on what you said about the usage.

mdedetrich commented 1 year ago

@pjfanning So it took me 10 minutes to do what I suggested, PR is already open (see https://github.com/apache/incubator-pekko-connectors/pull/74).