prettier-java
prettier-java copied to clipboard
jhipster
violates JLS formatting for type annotations
prettier violates the JLS which states that a type annotation should be just before the type
Prettier-Java 0.5.1
Input:
private @Nullable Neo4j neo4j;
Output:
@Nullable
private Neo4j neo4j;
Expected behavior:
private @Nullable Neo4j neo4j;
https://docs.oracle.com/javase/specs/jls/se12/html/jls-9.html#jls-9.7.4
Interesting, it seems like prettier-java would have no way to tell whether annotated fields are declaration annotations, type annotations, or both:
For example, given the field declaration:
@Foo int f;
@Foo is a declaration annotation on f if Foo is meta-annotated by @Target(ElementType.FIELD), and a type annotation on int if Foo is meta-annotated by @Target(ElementType.TYPE_USE). It is possible for @Foo to be both a declaration annotation and a type annotation simultaneously.
Later on it states:
It is customary, though not required, to write declaration annotations before all other modifiers, and type annotations immediately before the type to which they apply.
The current prettier-java behavior seems to match the JLS recommendation for declaration annotations, which I think is reasonable (assuming that prettier-java doesn't have enough information to distinguish between declaration and type annotations). I think declaration annotations are still much more prevalent, and putting annotations before modifiers also formats better when linebreaks are needed, for example:
@AnnotationNumberOne
@AnnotationNumberTwo
@AnnotationNumberThree
@AnnotationNumberFour
public MyClass myClass;
vs.
public @AnnotationNumberOne @AnnotationNumberTwo @AnnotationNumberThree @AnnotationNumberFour MyClass myClass;
(I'm not sure how you could even line-break this)
Yeah, type annotations aren't that common, honestly I only see it related to @Nullable other nullity-related annotations. To be honest you could just support it via checks for a handful of static ones, and provide functionality for people to provide a list. I'm just annoyed because I have another checker telling me I suck.
Yeah I feel your pain, we had some similar struggles at first with prettier clashing with other tools. Eventually we got rigorous about splitting our validation tools into syntactic (how the code looks) vs. semantic (what the code does). We decided to standardize on prettier for all syntactic validation, and disable anything else that was conflicting with it. And for semantic validation, we're trying to standardize on error-prone for everything (although we still have some other tools in the mix at the moment).
Given prettier's option philosophy, it doesn't seem like this would be worth adding an option for
ironic you should mention error prone, because that's what's checking this error
/home/xeno/IdeaProjects/pm/backend/module/util/model-graph/src/testFixtures/java/com/xenoterracide/ppm/util/modelgraph/Neo4jExtension.java:18: error: [AnnotationPosition] @Nullable is a TYPE_USE annotation, so should appear after modifiers and directly before the type.
private Neo4j neo4j;
^
(see https://google.github.io/styleguide/javaguide.html#s4.8.5-annotations)
Did you mean to remove this line?
1 error
We don't use any of the checks with tags = Style, but it's a bummer they're even adding those sorts of checks. That sort of check doesn't seem to meet their own criteria, even for a warning:
https://errorprone.info/docs/criteria
The warning should be for something that has the potential for significant impact. We want the warnings to be important enough so that when developers see them they take them seriously and often choose to fix them.
I can just disable it 🤷🏻♂️, I don't have more arguments than these ;)
One quick solution would be:
// prettier-ignore
private @Nullable Neo4j neo4j;
I can just disable it 🤷🏻♂️, I don't have more arguments than these ;)
Yeah given that prettier-java is in charge of all the annotation positions, it seems like that check will just keep badgering you about stylistic stuff you can't fix
perhaps not moving annotations out of that position would be a good workaround per #548 I don't notice this problem with getter/setters, so what's being done there?