You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "mdedetrich (via GitHub)" <gi...@apache.org> on 2024/03/29 08:50:31 UTC
[PR] Apply inspections [pekko-connectors]
mdedetrich opened a new pull request, #588:
URL: https://github.com/apache/pekko-connectors/pull/588
tbd
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "samueleresca (via GitHub)" <gi...@apache.org>.
samueleresca commented on code in PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#discussion_r1555022609
##########
elasticsearch/src/test/java/docs/javadsl/ElasticsearchV5Test.java:
##########
@@ -414,18 +414,16 @@ public void testUsingSearchParams() throws Exception {
searchParams.put("query", "{\"match_all\": {}}");
searchParams.put("_source", "[\"id\", \"a\", \"c\"]");
- List<TestDoc> result =
+ // These documents will only have property id, a and c (not
Review Comment:
Is the positioning of this comment correct? Worth removing the `(not` part in the context of this example?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#issuecomment-2041517236
I would suggest commit one by one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "samueleresca (via GitHub)" <gi...@apache.org>.
samueleresca commented on code in PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#discussion_r1555027821
##########
testkit/src/main/scala/org/apache/pekko/stream/connectors/testkit/CapturingAppender.scala:
##########
@@ -56,8 +56,8 @@ import org.slf4j.LoggerFactory
* the captured logging events are flushed to the appenders defined for the
* org.apache.pekko.actor.testkit.typed.internal.CapturingAppenderDelegate logger.
*
- * The flushing on test failure is handled by [[pekko.actor.testkit.typed.scaladsl.LogCapturing]]
- * for ScalaTest and [[pekko.actor.testkit.typed.javadsl.LogCapturing]] for JUnit.
+ * The flushing on test failure is handled by [[org.apache.pekko.stream.connectors.testkit.scaladsl.LogCapturing]]
+ * for ScalaTest and [[org.apache.pekko.stream.connectors.testkit.scaladsl.LogCapturing]] for JUnit.
Review Comment:
Should be `org.apache.pekko.stream.connectors.testkit.javadsl.LogCapturingJunit4`for Junit?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#issuecomment-2041517470
Another thing is case class can be extended in Java, this is not binary compatible
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#issuecomment-2041518785
> Another thing is case class can be extended in Java
It doesn't matter if it's Java or Scala, case classes shouldn't ever be extended because the usage of case class implies certain properties that can be broken if a user overrides them.
If a case class is intended to be overridden, you should use a normal class instead.
> this is not binary compatible
The entire Pekko 1.1.x series is not binary compatible with Pekko 1.0.x, that's why this is being done now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#issuecomment-2053972343
FYI don't merge this PR, I am in the process of splitting out the pure formatting changes into a separate commit so I can add it to `.git-blame-ignore-revs` and also cherry pick it into 1.0.x, stay tuned!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "samueleresca (via GitHub)" <gi...@apache.org>.
samueleresca commented on code in PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#discussion_r1555020942
##########
csv/src/main/scala/org/apache/pekko/stream/connectors/csv/impl/CsvToMapJavaStage.scala:
##########
@@ -23,7 +23,8 @@ import pekko.stream.stage.{ GraphStage, GraphStageLogic, InHandler, OutHandler }
import pekko.util.ByteString
/**
- * Internal Java API: Converts incoming {@link Collection}<{@link ByteString}> to {@link java.util.Map}<String, ByteString>.
+ * Internal Java API: Converts incoming [[ju.Collection]] containing a [[ByteString]] to [[ju.Map]] with [[String]] as
+ * as key.
Review Comment:
minor: duplicate `as`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "samueleresca (via GitHub)" <gi...@apache.org>.
samueleresca commented on PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#issuecomment-2041558672
I had a quick skim through and left few minor comments. Is this change a good candidate for the `.git-blame-ignore-revs`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#issuecomment-2041581695
@samueleresca Thanks, I have just force pushed with all of the changes fixed. I guess that technically speaking some of the changes can be added to `.git-blame-ignore-revs` but I would have to manually seperate the pure syntax changes with the non syntax ones and that would take a bit.
I can do this once all of the other issues with the PR have been resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#issuecomment-2041522492
> I think we should add annotations. @INTERNAL_API and/Or @doNotInherent in 1.1 cross the projects, and mark it final in 1.2 or 2.0 what ever.
>
>
> In pekko, there are some classes which is not annotated with these annotations but just comments internal api.sorry, typing from phone
Alpakka has never had any prpper binary compatibility guarantees, it's simply not practical with a monorepo with 20+ integrations and we inherited that.
Due to the fact that we have done major underlying library updates causing breaking changes to many connectors already, the entire release will be classed as breaking.
The plan is that once that release is made, we will start splitting off the connectors into their own repos so we don't get the issues stemming from a monorepo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#issuecomment-2041527339
Okay, but we should add those annotations in pekko core project
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "samueleresca (via GitHub)" <gi...@apache.org>.
samueleresca commented on code in PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#discussion_r1555022678
##########
elasticsearch/src/test/java/docs/javadsl/ElasticsearchV5Test.java:
##########
@@ -414,18 +414,16 @@ public void testUsingSearchParams() throws Exception {
searchParams.put("query", "{\"match_all\": {}}");
searchParams.put("_source", "[\"id\", \"a\", \"c\"]");
- List<TestDoc> result =
+ // These documents will only have property id, a and c (not
Review Comment:
Same for `*V7Test`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "samueleresca (via GitHub)" <gi...@apache.org>.
samueleresca commented on code in PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#discussion_r1555026153
##########
hdfs/src/main/scala/org/apache/pekko/stream/connectors/hdfs/model.scala:
##########
@@ -65,7 +65,7 @@ object HdfsWritingSettings {
val default = new HdfsWritingSettings(
overwrite = true,
newLine = false,
- lineSeparator = System.getProperty("line.separator"),
+ lineSeparator = java.lang.System.lineSeparator(),
Review Comment:
Should be `System.lineSeparator`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#issuecomment-2041520036
I think we should add annotations. @INTERNAL_API and/Or @doNotInherent in 1.1 cross the project, and mark it final in 1.2 or 2.0 what ever.
In pekko, there are some classes which is not annotated with these annotations but just comments internal api.sorry, typing from phone
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#issuecomment-2041582263
> Okay, but we should add those annotations in pekko core project
We can do this once we start splitting off the connectors into their own repo's as it will make sense then. The idea is that this release of pekko-connectors will the last of the "massive breaking changes"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Apply inspections [pekko-connectors]
Posted by "samueleresca (via GitHub)" <gi...@apache.org>.
samueleresca commented on code in PR #588:
URL: https://github.com/apache/pekko-connectors/pull/588#discussion_r1555024111
##########
google-cloud-pub-sub/src/test/java/docs/javadsl/ExampleUsageJava.java:
##########
@@ -108,30 +108,25 @@ private static void example() throws NoSuchAlgorithmException, InvalidKeySpecExc
Sink<AcknowledgeRequest, CompletionStage<Done>> ackSink =
GooglePubSub.acknowledge(subscription, config);
- subscriptionSource
- .map(
- message -> {
- // do something fun
- return message.ackId();
- })
+ // do something fun
+ subscriptionSource
+ .map(ReceivedMessage::ackId)
.groupedWithin(1000, Duration.ofMinutes(1))
- .map(acks -> AcknowledgeRequest.create(acks))
+ .map(AcknowledgeRequest::create)
.to(ackSink);
// #subscribe
// #subscribe-source-control
- Source.tick(Duration.ofSeconds(0), Duration.ofSeconds(10), Done.getInstance())
+ // do something fun
Review Comment:
The `do something fun line` has been duplicated in this case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org