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

Port imports and dependencies of CSV Samples to Pekko #6

Closed
wants to merge 4 commits into from

Conversation

reypader
Copy link
Contributor

Fixes #5

Copy link

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

Quickly found a style thing, will review the PR properly later.

import akka.stream._
import akka.stream.scaladsl.{ Sink, Source }
import akka.util.ByteString
import org.apache.pekko.Done

Choose a reason for hiding this comment

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

@reypader Pekko uses a root nested imports style, that is

import org.apache.pekko
import pekko.Done
import pekko.actor._
etc etc

Would you be able to update the PR to this style for all .scala source files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been trying to play around with my formatters and scalafmt. I copied over scalafmt from pekko-connectors project even. No luck. I can't seem to find formatter config that would automatically do this for me. Could you show me how?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like you have a similar thread on this specific topic apache/pekko#414

Since this project is separate from incubator-pekko maybe it would be easier to enable scalafix here?

Choose a reason for hiding this comment

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

Regarding scalafmt, this can be done later. For now I would just manually do the change I suggested

@@ -31,8 +31,7 @@

<logger name="org.apache" level="WARN"/>
<logger name="kafka" level="WARN"/>
<logger name="akka" level="WARN"/>
<logger name="akka.kafka.benchmarks" level="INFO"/>
<logger name="org.apache.pekko" level="WARN"/>
<logger name="org.apache.kafka.common.utils.AppInfoParser" level="ERROR"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add back <logger name="org.apache.pekko.kafka.benchmarks" level="INFO"/> ?

@pjfanning
Copy link
Contributor

done in other PRs

@pjfanning pjfanning closed this Dec 17, 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.

Port CSV-to-Kafka Samples to Pekko
3 participants