Skip to content
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

Change formatter to align parameters in method declaration #14783

Closed

Conversation

bziobrowski
Copy link
Contributor

@bziobrowski bziobrowski commented Jan 9, 2025

PR changes IJ formatter to align multiline method declaration parameters.
Current settings make multi-parameter declarations hard to read because everything is crammed into as few lines as possible.
With alignment enabled, declaration such as :

public MultistageGroupByExecutor(int[] groupKeyIds, AggregationFunction[] aggFunctions, int[] filterArgIds,
      int maxFilterArgId, AggType aggType, boolean leafReturnFinalResult, DataSchema resultSchema,
      Map<String, String> opChainMetadata, @Nullable PlanNode.NodeHint nodeHint) {

can be reformatted as

public MultistageGroupByExecutor(int[] groupKeyIds,
                                 AggregationFunction[] aggFunctions,
                                 int[] filterArgIds,
                                 int maxFilterArgId, AggType aggType,
                                 boolean leafReturnFinalResult,
                                 DataSchema resultSchema,
                                 Map<String, String> opChainMetadata,
                                 @Nullable PlanNode.NodeHint nodeHint) {

or

public MultistageGroupByExecutor(
      int[] groupKeyIds,
      AggregationFunction[] aggFunctions,
      int[] filterArgIds,
      int maxFilterArgId, AggType aggType,
      boolean leafReturnFinalResult,
      DataSchema resultSchema,
      Map<String, String> opChainMetadata,
      @Nullable PlanNode.NodeHint nodeHint) {

or differently - with multiple parameters per line - but with first parameter always aligned.

@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.83%. Comparing base (59551e4) to head (0b7bfe4).
Report is 1557 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #14783      +/-   ##
============================================
+ Coverage     61.75%   63.83%   +2.08%     
- Complexity      207     1611    +1404     
============================================
  Files          2436     2704     +268     
  Lines        133233   150977   +17744     
  Branches      20636    23320    +2684     
============================================
+ Hits          82274    96378   +14104     
- Misses        44911    47381    +2470     
- Partials       6048     7218    +1170     
Flag Coverage Δ
custom-integration1 100.00% <ø> (+99.99%) ⬆️
integration 100.00% <ø> (+99.99%) ⬆️
integration1 100.00% <ø> (+99.99%) ⬆️
integration2 0.00% <ø> (ø)
java-11 56.24% <ø> (-5.47%) ⬇️
java-21 63.73% <ø> (+2.11%) ⬆️
skip-bytebuffers-false 63.82% <ø> (+2.08%) ⬆️
skip-bytebuffers-true 63.71% <ø> (+35.98%) ⬆️
temurin 63.83% <ø> (+2.08%) ⬆️
unittests 63.83% <ø> (+2.08%) ⬆️
unittests1 56.27% <ø> (+9.38%) ⬆️
unittests2 34.12% <ø> (+6.39%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jackie-Jiang
Copy link
Contributor

This is huge change which can potentially cause changes for every single method, and make PRs very hard to review. The current style setting honors line breaks, so if you strongly prefer the new format, it should allows you to do that (i.e. IDE reformat won't change it).
Let's not change the style sheet because this change has too high impact, and it is personal preference.

@bziobrowski
Copy link
Contributor Author

Hmm, it actually is possible to split long param list into multiple lines now.
That wasn't allowed the last time I checked.
I'm ok with closing the PR then.

@bziobrowski bziobrowski closed this Jan 9, 2025
@bziobrowski bziobrowski deleted the change_method_param_formatting branch January 9, 2025 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants