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

AVRO-4053: doc consistency in velocity templates #3150

Merged

Conversation

opwvhk
Copy link
Contributor

@opwvhk opwvhk commented Sep 6, 2024

What is the purpose of the change

AVRO-4053: Improve doc consistency in SpecificRecord

Fixes inconsistencies in generated doc comments.

Verifying this change

This change adds tests and can be verified as follows:

  1. Verify the updates velocity templates
  2. Check that the updated file simple_record.avsc (used by many tests) could but does not cause problems
  3. Review the new test docsAreEscaped_avro4053 in org.apache.avro.compiler.specific.TestSpecificCompiler

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@opwvhk opwvhk requested a review from martin-g September 6, 2024 19:41
@github-actions github-actions bot added the Java Pull Requests for Java binding label Sep 6, 2024
@@ -17,14 +17,19 @@
*/
package org.apache.avro.compiler.specific;

import static org.hamcrest.CoreMatchers.equalTo;
Copy link
Member

Choose a reason for hiding this comment

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

IMO static imports are usually before the non-static ones. Please update your IDE settings to not do these changes automatically.

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 see both in the codebase (I checked classes I did not alter).

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 appears the default differs per IDE: https://checkstyle.sourceforge.io/checks/imports/importorder.html (Eclipse sorts static imports first, IntelliJ last). I did not find a definition in the codebase.

I can add it to .editorconfig, but only using two IntelliJ-specific properties ij_java_imports_layout and ij_java_layout_static_imports_separately. Shall I do that?

Copy link
Member

Choose a reason for hiding this comment

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

Another way is to tell IDEA to not format the lines which are not edited

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 took a while, but I've delved into this a bit: most other source files use a "java first, then others and static last" approach. However, many tests use a "static first, then java, then others" approach. Also, many files contain missorted imports.

It's probably best to simply stick to a single, preferably common, standard.

For that, there are 4 major choices: CheckStyle, Google, Eclipse, and JetBrains, with the latter two modified to not allow * imports (this mostly affects the JetBrains style).

Using these code styles (and two custom styles inferred from the code) yield the following number of changed files:

Style .editorconfig approximation Files changed
Custom 1 java.**,javax.**,|,*,|,$* 389 (51%)
Eclipse $*,|,java.**,|,javax.**,|,org.**,|,com.**,|,* 391 (51%)
JetBrains *,|,java.**,javax.**,|,$* 454 (60%)
Custom 2 $*,|,*,|,java.**,javax.** 516 (68%)
Google $*,|,* 548 (72%)
CheckStyle *,$* 554 (73%)

As you can see from the number (and percentage) of changed files, if the codebase ever followed an import code style, this was a very long time ago.

Taking a different approach, finding static imports and JDK (java/javax) imports among all and 1st imports, does describe a pattern:

  • there are 233 files with static imports, of which 230 (99%) sort them before non-static imports
  • there are 635 files with JDK imports, of which 109 (17%) sort them before other (non-static) imports

Using these two metrics, we should:

  • sort static imports before non-static imports
  • sort java/javax imports after other imports

This yields the code style labelled "Custom 2" in the table above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, thanks so much for the investigation! Can we put this "import style" issue into a JIRA and do it separately!?

We've alway just ... well, never bothered imposing an import order, but I can see that it's just an unnecessary pain because devs expect autoformat to leave unmodified code.

Haha, just weighing in though: we've chosen spotless:apply to format our code in the checks, and my preference is to do whatever spotless does by default :D That ends up being the Google style in the table!

Copy link
Member

Choose a reason for hiding this comment

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

Definitely don't format all files in this PR! :-)
As I said earlier - IMO the best would be to not sort the imports at all. Just use the current style for the modified file.

@RyanSkraba
Copy link
Contributor

Thanks for addressing Martin's comments! I've gone through this PR and it all looks good to me ... except that I haven't quite finished thoroughly reviewing that Regex construction 😆 My apologies, I'm committing to finishing this tomorrow!

@opwvhk opwvhk force-pushed the AVRO-4053-doc-consistency-velocity-templates branch from 97b118f to e21a94c Compare September 27, 2024 08:14
@opwvhk
Copy link
Contributor Author

opwvhk commented Sep 27, 2024

I've taken the liberty of taking the import order config out of this PR, and started a discussion on the dev mailing list.

Copy link
Contributor

@RyanSkraba RyanSkraba left a comment

Choose a reason for hiding this comment

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

Hey, thanks for your patience! I reviewed this and it looks like it covers all the cases necessary for generated code comments (including your thorough work on annotations). Thanks so much! LGTM

@opwvhk opwvhk merged commit 84bc732 into apache:main Oct 4, 2024
8 checks passed
@opwvhk opwvhk deleted the AVRO-4053-doc-consistency-velocity-templates branch October 4, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java Pull Requests for Java binding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants