-
Notifications
You must be signed in to change notification settings - Fork 0
impose AutoFormat convention over configuration principle ; promote checkstyle config burden to convention, use org.openrewrite.java.format.AutoFormat
#2
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
base: main
Are you sure you want to change the base?
Conversation
…cessaryLocalBeforeBranch
# Conflicts: # pom.xml
convention over configuration principle ; promote checkstyle config burden to convention following org.openrewrite.java.format.AutoFormatconvention over configuration principle ; promote checkstyle config burden to convention, use org.openrewrite.java.format.AutoFormat
| @SuppressWarnings("unchecked") | ||
| T t = (T) node; | ||
| @Override public Object visitApexNode(ApexNode<?> node, Object data) { | ||
| @SuppressWarnings("unchecked") T t = (T) node; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the intended format @timtebeek or a bug? inlined this looks quite off. Thanks for considering.
The documentation is quite broad:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what version you ran; we had a bug fixed and released Wednesday.
| } | ||
|
|
||
| switch (node.getMethodName().toLowerCase(Locale.ROOT)) { | ||
| case "insert": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My contribution to PMD is minimal (1 accepted PR and a few comments), so I'm definitely not the right person to decide the code style for PMD.
That being said, this is aligned with Sun's code style: https://checkstyle.sourceforge.io/styleguides/sun-code-conventions-19990420/CodeConventions.doc6.html#a468
and for projects that use 4 space indentation it IMHO makes sense. On the other hand, Google code style indents the cases: https://google.github.io/styleguide/javaguide.html#s4.8.4-switch (but it also uses 2 space indentation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sun's code style
crazy to see, thanks for that.
Yes of course we not going to decide, we only can suggest wether auto fix is a nice thing to have to rather we want to fix these trivial issues by hand like its currently approached. That beging sad the current setup feels like 2015 but after a decade of fixing the silliness myself, finally theres an app for that imposing convention of configuration.
Is the convention of configuration principle something you would appreciate in order to improve the dev exp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's how PMD's code style looks like. We explictly say, that cases should not be indented:
Whether this checkstyle rule works reliably, is another topic (If you search, you already find indented cases in PMD sources, that checkstyle doesn't complain about).
So - in short: you are changing PMD's code style... was this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this intended
of course not.
If the cost of all config burden is vanished for the price to waive some beloved spaces before array init, as example, the tradeoff would be quite simple, as least for me.
Its a lot to talk about this and then. When the answer is always we use some others convention the discussion seems boringly done.
So its up to you do balance the price for the caseIndent over the cost for the current configuration imposed.
A simple convention, ignoring its randomly details, makes the best, or even broken, config obsolete.
impose AutoFormat
convention over configurationprinciple ; promotecheckstyleconfig burden to convention, useorg.openrewrite.java.format.AutoFormatRelated issues
Ready?
./mvnw clean verifypasses (checked automatically by github actions)