google/certificate-transparency-java

Code quality: Run google-java-format, style guide compliance

Closed this issue · 6 comments

Deleted user commented

So, I quickly upgraded fmt-maven-plugin to use google-java-format 1.3, and ran it against HEAD and the following list of files ended up being changed:

  • src/main/java/org/certificatetransparency/ctlog/CTLogClient.java
  • src/main/java/org/certificatetransparency/ctlog/CertificateInfo.java
  • src/main/java/org/certificatetransparency/ctlog/CertificateTransparencyException.java
  • src/main/java/org/certificatetransparency/ctlog/LogEntry.java
  • src/main/java/org/certificatetransparency/ctlog/LogInfo.java
  • src/main/java/org/certificatetransparency/ctlog/LogSignatureVerifier.java
  • src/main/java/org/certificatetransparency/ctlog/MerkleAuditProof.java
  • src/main/java/org/certificatetransparency/ctlog/MerkleTreeLeaf.java
  • src/main/java/org/certificatetransparency/ctlog/ParsedLogEntry.java
  • src/main/java/org/certificatetransparency/ctlog/ParsedLogEntryWithProof.java
  • src/main/java/org/certificatetransparency/ctlog/PreCert.java
  • src/main/java/org/certificatetransparency/ctlog/PrecertChainEntry.java
  • src/main/java/org/certificatetransparency/ctlog/SignedEntry.java
  • src/main/java/org/certificatetransparency/ctlog/SignedTreeHead.java
  • src/main/java/org/certificatetransparency/ctlog/TimestampedEntry.java
  • src/main/java/org/certificatetransparency/ctlog/UnsupportedCryptoPrimitiveException.java
  • src/main/java/org/certificatetransparency/ctlog/X509ChainEntry.java
  • src/main/java/org/certificatetransparency/ctlog/comm/HttpInvoker.java
  • src/main/java/org/certificatetransparency/ctlog/comm/HttpLogClient.java
  • src/main/java/org/certificatetransparency/ctlog/comm/LogCommunicationException.java
  • src/main/java/org/certificatetransparency/ctlog/serialization/CTConstants.java
  • src/main/java/org/certificatetransparency/ctlog/serialization/CryptoDataLoader.java
  • src/main/java/org/certificatetransparency/ctlog/serialization/Deserializer.java
  • src/main/java/org/certificatetransparency/ctlog/serialization/InvalidInputException.java
  • src/main/java/org/certificatetransparency/ctlog/serialization/SerializationException.java
  • src/main/java/org/certificatetransparency/ctlog/serialization/Serializer.java
  • src/main/java/org/certificatetransparency/ctlog/utils/PrintCertificates.java
  • src/main/java/org/certificatetransparency/ctlog/utils/UploadCertificate.java
  • src/main/java/org/certificatetransparency/ctlog/utils/VerifySignature.java
  • src/test/java/org/certificatetransparency/ctlog/CertificateInfoTest.java
  • src/test/java/org/certificatetransparency/ctlog/LogInfoTest.java
  • src/test/java/org/certificatetransparency/ctlog/LogSignatureVerifierTest.java
  • src/test/java/org/certificatetransparency/ctlog/TestData.java
  • src/test/java/org/certificatetransparency/ctlog/comm/HttpLogClientTest.java
  • src/test/java/org/certificatetransparency/ctlog/serialization/TestSerializer.java

I'm happy to submit these changes, but given the size of it is there a preferred way you'd like it broken down? Also it would be good to have the formatting as part of the build cycle - something that isn't yet easy (see this issue).

codingllama commented

I'm happy to take the formatting changes in a single batch. I actually prefer it that way, instead of broken down into multiple PRs.

I'd like to have google-java-format running as part of the build, as you suggested, but I haven't looked into how much trouble it is. In any case, having an easy way to run it, even if it's manual, beats what we have now. There isn't a lot of dev going on in this repo, so the code should remain relatively stable.

Deleted user commented

With regards to fixing this as part of the build process, I see a few options:

  • Provide a maven plugin to google-java-format, which would resolve google/google-java-format#69
  • Use Coveo's maven plugin, it's MIT licensed (which appears on the notice list for Google's approved licenses).
  • Look at switching over to Bazel instead of Maven - Gerrit has incorporated the java formatter as part of their build process. I'm completely unfamiliar with Bazel, so this to me appears the most difficult.
  • There could be other tooling inside of Google I'm not privvy to that may also be considered for open sourcing.

I'm leaning towards using Coveo's tooling for now, as it exists and appears to work and I think I can manipulate it to be incorporated during the compile phase. Any thoughts @codingllama?

codingllama commented

fmt-maven-plugin seems to be the way to go. No reason to write our own, nor is this a strong enough reason to change build systems IMO.

Deleted user commented

I guess we can close this ticket?

codingllama commented

We're well covered on the google-java-format part. Thanks for contributing it!

There may be more to be done on style guide compliance, but I'm happy enough to have the ticket closed.