Issue #15098: Indentation of switch block when no braces is now validated #16721
Conversation
|
GitHub, generate report |
|
@romani I did a refork, and now report generation is working perfectly. |
|
lineWrappingIndentation=4 file looks like using google style, so indentation is valid, am I miss somthing ? |
|
@pnagy-cldr , @mohitsatr , is this potential fix for lambda in case ? |
I based the logic in the fix on using To align with how Checkstyle handles blocks inside switch lambdas with curlies, I tested the same code but added braces /*
* Copyright Camunda Services GmbH and/or licensed to Camunda Services GmbH under
* one or more contributor license agreements. See the NOTICE file distributed
* with this work for additional information regarding copyright ownership.
* Licensed under the Camunda License 1.0. You may not use this file
* except in compliance with the Camunda License 1.0.
*/
package io.camunda.authentication.config;
import io.camunda.security.configuration.SecurityConfiguration;
import io.camunda.security.entity.AuthenticationMethod;
public final class AuthenticationProperties {
public static final String METHOD = "camunda.security.authentication.method";
public static final String API_UNPROTECTED = "camunda.security.authentication.unprotected-api";
private AuthenticationProperties() {}
public static String getUnprotectedApiEnvVar() {
return API_UNPROTECTED.replace(".", "_").replace("-", "").toUpperCase();
}
public static void applyToSecurityConfig(
final SecurityConfiguration securityConfig, final String key, final Object value) {
switch (key) {
case METHOD -> {
AuthenticationMethod.parse(String.valueOf(value))
.ifPresent(securityConfig.getAuthentication()::setMethod);
}
case API_UNPROTECTED -> {
securityConfig
.getAuthentication()
.setUnprotectedApi(Boolean.parseBoolean(String.valueOf(value)));
}
default -> {}
}
}
}So I followed that same logic in the fix. WDYT? |
| System.out.println("Test"); | ||
| System.out.println("Another statement"); | ||
| } | ||
| default -> System.out.println("test"); |
There was a problem hiding this comment.
@Aziz-755 just curious, If we specify different values for basicOffset and lineWrappingIndentation, it would cause the indentation of cases children to be misaligned across the switch block, right?
There was a problem hiding this comment.
@mohitsatr So if the case blocks were of this format:
case 1 -> {
// some code
}or of this format:
case 1 ->
// some codeThey will be aligned ( here they are using the basicOffset value )
However if the case was like this:
case 1
-> // lineWrapping used here
int x; // basicOffset used hereFor this case it will be misaligned in regard of the other cases.
There was a problem hiding this comment.
case 1 ->
// should be lineWrapping, not basicOffsetaccording to docs :
Basic offset indentation is used for indentation inside code blocks( {} ). For any lines that span more than 1, line wrapping indentation is used for those lines after the first
since there is no code block in this case, we must not use basicOffset property. look at similar cases
void function1(Runnable x) { //indent:2 exp:2
Runnable r1 = () -> { //indent:4 exp:4
x.run(); //indent:6 exp:6
}; //indent:4 exp:4
Runnable r3 = () -> //indent:4 exp:4
x.run(); //indent:8 exp:8
There was a problem hiding this comment.
Thanks for the information. I have adjusted the implementation and used lineWrappingIndentation instead
b73eff4 to
b5dfd2b
Compare
|
GitHub, generate report |
|
After some discussion and code refinement, I believe the current implementation is in a good state. Upon reviewing the report, the remaining violations appear to be false positives caused by the existing switch statement usage |
|
@Aziz-755 can we end them in this PR. false-negatives are better than fals-positives. it's okay if you feel stuck. it's not you, The check has design flaws. |
|
what is strange in this diff report that that there is no removal of violations and bunch is added. users should see reduction of bad violations, but we looks like add more. |
|
@romani Actually the issue #15098 is about a false negative, so the fix here should in fact add a new violations. The newly introduced issues in the diff report are also due to an already existing false positives about the usage of |
|
yes, you are right. new violations are expected, as you add validation for switches. it sounds like this project use +2 for indentaiton, |
b5dfd2b to
2776652
Compare
|
GitHub, generate report |
2776652 to
c854458
Compare
|
GitHub, generate report |
c854458 to
160e295
Compare
|
GitHub, generate report |
160e295 to
38b6417
Compare
|
Github, generate report for Indentation/all-examples-in-one |
|
Report for Indentation/all-examples-in-one: |
|
GitHub, generate report |
|
@romani @mohitsatr I addressed the false positives related to switch expressions when they are used with line wrapping. For example, see the nested switch starting at line 297: ( I think the false positives I fixed are the same as in this issue: #16762 ) 295 return switch (resource) {
296 case brokers ->
297 switch (subResource) {
298 // POST /cluster/brokers/1/partitions/2
299 case partitions ->
300 ClusterApiUtils.mapOperationResponse(
301 requestSender
302 .joinPartition(
303 new JoinPartitionRequest(
304 MemberId.from(String.valueOf(resourceId)),
305 subResourceId,
306 priority,
307 dryRun))
308 .join());
309 case brokers, changes -> new ResponseEntity<>(HttpStatusCode.valueOf(404));
310 };Initially, this false positive was also causing the new logic (fixing the missing violations for case/default elements without curly braces) to incorrectly report violations. |
There was a problem hiding this comment.
Great job. we actually removed all false-positives.
one last thing, we have only checked the regression on camunda project. Hbase also uses google-style-format, we need to check regression on that too unless there is a reason not to.
@romani
|
@Aziz-755 , please fix CI violations. |
...tyle/test/chapter4formatting/rule4841indentation/InputSingleSwitchStatementWithoutCurly.java
Show resolved
Hide resolved
.../checks/indentation/indentation/InputIndentationCheckSingleSwitchStatementsWithoutCurly.java
Outdated
Show resolved
Hide resolved
|
GitHub, generate report |
|
Execution in mode "New module config:" |
80d3eb3 to
d1c81cb
Compare
|
The CI check is complaining that I am misspelling "prefs" however, I am not using any word like that in my code. |
|
Github, generate report for Indentation/all-examples-in-one |
|
Report for Indentation/all-examples-in-one: |
d1c81cb to
bce2ceb
Compare
|
I have no idea ....very strange |
bce2ceb to
d3b1012
Compare
fixes: #15098 , #16762
This PR reintroduces the changes from the original PR (#16647), which was closed due to a required refork of the repository.
New module config: https://gist.githubusercontent.com/mohitsatr/937e10572dd412961b61c77577619dff/raw/7a47240f8793e7335adbd81c2806b375f0394815/google_checks_indentation.xml
---Diff-- Regression config: https://gist.githubusercontent.com/mohitsatr/937e10572dd412961b61c77577619dff/raw/7a47240f8793e7335adbd81c2806b375f0394815/google_checks_indentation.xml
Diff Regression projects: https://gist.githubusercontent.com/mohitsatr/68b0817ac52ff7773d35585ee12fc1cc/raw/9ba8e4fb324e1daf589874c6992f2aa87b03079a/list-of-projects.properties