AshishGhodvinde commented on PR #5721: URL: https://github.com/apache/datafusion-comet/pull/5721#issuecomment-5557035125
Thanks for the suggestion! I looked into `rewrite.insertBraces.minLines` using the pinned Scalafmt 3.6.1 version. I tested both `minLines = 1` and `minLines = 2` with `IfBraceChecker` still enabled. ### What I found - **`minLines = 1`** does fix the `IfBraceChecker` violations, but it causes quite a lot of unrelated formatting changes: **295 Scala files** were modified, with around **6,158 lines of churn**, and approximately **565 single-expression method bodies** gained braces. - **`minLines = 2`** reduces the churn somewhat (**249 files**, around **4,166 lines**), but it still leaves **4 `IfBraceChecker` violations** when scanning all relevant main, test, and Spark-version source directories. The `minLines = 2` violations are cases where Scalafmt wraps a single-statement `if`/`else` branch across a line break but doesn't insert braces. I also found that my initial `minLines = 2` check was incomplete because the default Scalastyle Maven configuration doesn't cover all of the repository's test and Spark-version source directories. I reran the check explicitly across those directories and reproduced the 4 violations. So after testing this more broadly, neither `minLines = 1` nor `minLines = 2` seems like a good fit without introducing significant formatting churn or, in the case of `minLines = 2`, still leaving the original issue unresolved. I haven't changed the PR implementation yet. For now, the `IfBraceChecker` change remains as-is. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
