Aiven-Open / tiered-storage-for-apache-kafka

RemoteStorageManager for Apache Kafka® Tiered Storage
Apache License 2.0
91 stars 19 forks source link

chore: reformat code #468

Closed jeqo closed 9 months ago

jeqo commented 9 months ago

Align with 4 spaces for continuation indent(line break) on next line and code re-alignment.

This commit will be hidden after merged.

AnatolyPopov commented 9 months ago

I guess it's 4 spaces for continuation indent(line break). Formatting in general looks good but can we enforce at least some of the things that were changed with e.g. CheckStyle? Otherwise we will have to reformat again at some point.

jeqo commented 9 months ago

@AnatolyPopov lineWrappingIndentation default is 4 -- do you mean to add this checkstyle config explicitly?

AnatolyPopov commented 9 months ago

@jeqo Yes but it does not seem to be enforced. If you change to more than 4 it does not fail. At the same time if it is enforced it fails in a little bit unexpected places like method arguments aligned in a column. Also there are other changes in this PR that need to be checked and added to config if possible, for example spaces like here https://github.com/Aiven-Open/tiered-storage-for-apache-kafka/pull/468/files#diff-dba09582f70dec690f01a4aa30b1cacbb174a1d10536db0c4be210bcabf58e3dR89

jeqo commented 9 months ago

@AnatolyPopov unfortunately, enforcing indentation goes into scenarios that are even impossible to fix without large changes, e.g. :

                result.add(Arguments.of(
                    Named.of("with disk-based cache",
                        Map.of(
                            "fetch.chunk.cache.class", DiskChunkCache.class.getCanonicalName(),
                            "fetch.chunk.cache.path", Files.createTempDirectory("cache").toString(),
                            "fetch.chunk.cache.size", "-1"
                        )
                    ),
                    readFully,
                    range)
                );

causes this checkstyle error:

[ant:checkstyle] [ERROR] aiven-open/tiered-storage-for-apache-kafka/core/src/test/java/io/aiven/kafka/tieredstorage/fetch/FetchChunkEnumerationSourceInputStreamClosingTest.java:126:21: ')' has incorrect indentation level 20, expected level should be 16. [Indentation]

which seems impossible to solve as the check fails to agree on the right indentation when there are nested parenthesis -- the only workaround I can find is to move nested object creation into another variable, but there are too many places to fix this.

About the other changes, I can't find any checkstyle config to map to -- seem to come from IDE reformatting. I can take those out if needed.

The only approach I can see is that we either keep it relaxed and periodically re-align any divergence (as this PR), or use a proper formatter (e.g. spotless+google-java-format) which matches our checkstyle config (this is the tricky part I foresee, mostly import order).

AnatolyPopov commented 9 months ago

Ok, for now lets add at least <property name="tokens" value="ARRAY_INIT"/> in WhitespaceAround module for the spaces I mentioned before at least. And the rest we will figure out later.