cadonna commented on a change in pull request #10428:
URL: https://github.com/apache/kafka/pull/10428#discussion_r612321725
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -573,23 +561,27 @@ public void resetOffsetsTo(final Consumer<byte[], byte[]>
client,
return topicPartitionAndOffset;
}
- private Map<TopicPartition, Long> checkOffsetRange(final
Map<TopicPartition, Long> inputTopicPartitionsAndOffset,
+ private Map<TopicPartition, Long> checkOffsetRange(
+ final
Map<TopicPartition, Long> inputTopicPartitionsAndOffset,
final
Map<TopicPartition, Long> beginningOffsets,
final
Map<TopicPartition, Long> endOffsets) {
final Map<TopicPartition, Long> validatedTopicPartitionsOffsets = new
HashMap<>();
- for (final Map.Entry<TopicPartition, Long> topicPartitionAndOffset :
inputTopicPartitionsAndOffset.entrySet()) {
+ for (final Map.Entry<TopicPartition, Long> topicPartitionAndOffset :
inputTopicPartitionsAndOffset
+ .entrySet()) {
Review comment:
Also this seems weird. I think this is because the line is 121
characters long.
##########
File path: eclipse-formatter.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<profiles version="1">
+<profile kind="CodeFormatterProfile" name="ApacheKafka" version="1">
+ <!-- Import ordering -->
Review comment:
Would it be possible to have only the import ordering in the formatter
file and adding the other formatting rules incrementally?
##########
File path: eclipse-formatter.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<profiles version="1">
+<profile kind="CodeFormatterProfile" name="ApacheKafka" version="1">
+ <!-- Import ordering -->
+ <setting
id="org.eclipse.jdt.core.formatter.blank_lines_between_import_groups"
value="1"/>
+ <setting id="org.eclipse.jdt.core.formatter.blank_lines_before_imports"
value="1"/>
+ <setting id="org.eclipse.jdt.core.formatter.blank_lines_after_imports"
value="1"/>
+
+ <!-- Line -->
+ <setting id="org.eclipse.jdt.core.formatter.lineSplit" value="120"/>
+ <setting id="org.eclipse.jdt.core.formatter.comment.line_length"
value="120"/>
Review comment:
http://kafka.apache.org/coding-guide.html says:
"There is not a maximum line length (certainly not 80 characters, we don't
work on punch cards any more), but be reasonable."
In Streams, we prefer a maximum line length of 120 but we do not enforce it
with checkstyle (at least I could not find a rule for it). While I do not like
long lines, I also think that sometimes there are good reasons to exceed the
limit. So I would not enforce it in the formatter.
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -573,23 +561,27 @@ public void resetOffsetsTo(final Consumer<byte[], byte[]>
client,
return topicPartitionAndOffset;
}
- private Map<TopicPartition, Long> checkOffsetRange(final
Map<TopicPartition, Long> inputTopicPartitionsAndOffset,
+ private Map<TopicPartition, Long> checkOffsetRange(
+ final
Map<TopicPartition, Long> inputTopicPartitionsAndOffset,
final
Map<TopicPartition, Long> beginningOffsets,
final
Map<TopicPartition, Long> endOffsets) {
Review comment:
I think that does not conform to our code style. I think it should look
like:
```suggestion
private Map<TopicPartition, Long> checkOffsetRange(final
Map<TopicPartition, Long> inputTopicPartitionsAndOffset,
final
Map<TopicPartition, Long> beginningOffsets,
final
Map<TopicPartition, Long> endOffsets) {
```
##########
File path: eclipse-formatter.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
Review comment:
I guess we need to the license header in this file similarly as in
`checkstyle.xml`.
##########
File path: README.md
##########
@@ -207,6 +207,20 @@ You can run checkstyle using:
The checkstyle warnings will be found in
`reports/checkstyle/reports/main.html` and
`reports/checkstyle/reports/test.html` files in the
subproject build directories. They are also printed to the console. The build
will fail if Checkstyle fails.
+As of present, the auto-formatting configuration is working in progress.
Auto-formatting is automatically invoked for the modules listed below when the
'checkstyleMain' or 'checkstyleTest' task is run.
Review comment:
```suggestion
As of present, the auto-formatting configuration is work in progress.
Auto-formatting is automatically invoked for the modules listed below when the
'checkstyleMain' or 'checkstyleTest' task is run.
```
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -72,17 +74,15 @@
* <li>deleting any topics created internally by Kafka Streams for this
application</li>
* </ol>
* <p>
- * Do only use this tool if <strong>no</strong> application instance is
running.
- * Otherwise, the application will get into an invalid state and crash or
produce wrong results.
+ * Do only use this tool if <strong>no</strong> application instance is
running. Otherwise, the application will get
+ * into an invalid state and crash or produce wrong results.
Review comment:
I guess the line break here was inserted on purpose. At least in
Streams, we try to break after each sentence in javadocs. It would be odd if
the formatter would remove the line break.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
