AVRO-4053: doc consistency in velocity templates

[opwvhk]

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)

[martin-g]

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

[opwvhk]

I see both in the codebase (I checked classes I did not alter).

[opwvhk]

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?

[martin-g]

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

[opwvhk]

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.

[RyanSkraba]

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!

[martin-g]

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]

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]

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