Add OneStatementPerLineCheck to Checkstyle rules #33682
Conversation
cbuescher
added
>enhancement
:Delivery/Build
|
Pinging @elastic/es-core-infra |
|
For context: I opened this as a follow up to #33676 after I (or better my Eclipse IDE) stumbled into some offending double semicolons in import statements. Those would be complained about as well with this additional checkstyle rule. |
There was a problem hiding this comment.
I also opened a separate PR to separate this change: #33678
There was a problem hiding this comment.
Actually it wasn't reformatting, just my Auto-Save trailing whitespace removal.
There was a problem hiding this comment.
I also opened a separate PR for this change: #33681
cbuescher
force-pushed
the
one-statement-per-line
branch
from
2cdcb73 to
d3dd075
Compare
|
This job triggered CI during a migration of the master. Kicking off an additional build for you manually... Jenkins, test this please. |
This change adds the OneStatementPerLineCheck to our checkstyle precommit checks. This rule restricts the number of statements per line to one. The resoning behind this is that it is very difficult to read multiple statements on one line. People seem to mostly use it in short lambdas and switch statements in our code base, but just going through the changes already uncovered some actual problems in randomization in test code, so I think its worth it.
cbuescher
force-pushed
the
one-statement-per-line
branch
from
d3dd075 to
e74219d
Compare
nik9000
left a comment
nik9000
left a comment
There was a problem hiding this comment.
I kind of like the switch statements the old way but if we can't convince checkstyle that that is ok then I think this is a net win.
| import static org.elasticsearch.index.mapper.TypeParsers.parseTextField; | ||
|
|
||
| /** A {@link FieldMapper} for full-text fields with annotation markup e.g. | ||
| * |
There was a problem hiding this comment.
Could you extract the formatting changes to this file into another commit so I don't have to review them? I'd be happy with you just pushing it so long as all it does is remove trailing spaces.
There was a problem hiding this comment.
Sure, I didn't realize this did a reformatting. Will clean this up.
I can understand that for short lines, but I must say I think readability still suffers. Plus this rule fund the two leftovers in test I mentioned, I think its worth it. |
This change adds the OneStatementPerLineCheck to our checkstyle precommit checks. This rule restricts the number of statements per line to one. The resoning behind this is that it is very difficult to read multiple statements on one line. People seem to mostly use it in short lambdas and switch statements in our code base, but just going through the changes already uncovered some actual problems in randomization in test code, so I think its worth it.
This change adds the OneStatementPerLineCheck to our checkstyle precommit checks. This rule restricts the number of statements per line to one. The resoning behind this is that it is very difficult to read multiple statements on one line. People seem to mostly use it in short lambdas and switch statements in our code base, but just going through the changes already uncovered some actual problems in randomization in test code, so I think its worth it.
6 participants
This change adds the OneStatementPerLineCheck to our checkstyle precommit
checks. This rule restricts the number of statements per line to one. The
resoning behind this is that it is very difficult to read multiple statements on
one line. People seem to mostly use more than one statement per line in short
lambdas and switch statements in our code base, but just going through the
necessary changes this rule enforces already uncovered some actual problems
in randomization in test code, so I think its worth it.