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

Code Style Change - Method Parameters on new line from parentheses #5776

Closed
wants to merge 8 commits into from

Conversation

ericdecanini
Copy link
Contributor

@ericdecanini ericdecanini commented Apr 15, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Applies the codes style change to move (multiline) method parameters on a new line from both the opening and closing parentheses

Motivation and context

Closes #5771

See also #5644

I tried to isolate this change to the mentioned issue as much as possible, but it seems the autoformatter wanted to correct the inconsistencies that weren't on the current code style

I tested conflicts for this by merging this branch into the Tchap fork and this seemed to introduce only a few more easy-to-resolve conflicts than there would be when merging with develop

I figured if the forks perform the same code style change strategy as undertaken to make the changes on this PR, they can encounter even less conflicts. I will document this strategy later, but in a nutshell, that strategy is:

  1. Strip down the .editorconfig file to only the changes being made
[{*.gradle.kts,*.kt,*.kts,*.main.kts}]
ij_kotlin_method_parameters_new_line_after_left_paren = true
ij_kotlin_method_parameters_right_paren_on_new_line = true
  1. Reformat the modules with file mask .kt

Screenshots / GIFs

N/A

Tests

  • Checkout Tchap fork
  • Add the original element repo as upstream and fetch all the branches
  • Merge this branch with Tchap develop

You will see that there are a lot of conflicts, but most of these conflicts come from changes that are on our repo's develop and not the changes on this branch specifically. If you repeat the steps above but instead merge upstream/develop into origin/develop, you will see that most of those conflicts are still present and this branch adds only few.

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@github-actions
Copy link

github-actions bot commented Apr 15, 2022

Unit Test Results

114 files  ±0  114 suites  ±0   1m 26s ⏱️ -8s
202 tests ±0  202 ✔️ ±0  0 💤 ±0  0 ±0 
678 runs  ±0  678 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 00e5f9c. ± Comparison against base commit 948566c.

♻️ This comment has been updated with latest results.

@ericdecanini ericdecanini requested review from bmarty and ouchadam April 15, 2022 15:47
@ericdecanini ericdecanini marked this pull request as ready for review April 15, 2022 15:48
inflater: LayoutInflater,
container: ViewGroup?,
savedInstanceState: Bundle?
inflater: LayoutInflater,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like these formatting changes could be done separately from the parameter changes (would also help reduce the noise is we're aiming to incrementally change the formatting rules)

Copy link
Contributor Author

@ericdecanini ericdecanini Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried isolating this change as much as possible, but I found that we can't do "I only want to reformat this change". Even if you removed all other lines from editorconfig, the IDE wants to follow each rule as either true or false.

In this case, this is a case of us having continuation indents enabled. If we disabled it, the rest of the project that is using continuation indents will change.

This is a consequence of us having inconsistent formatting throughout the project

Through trial and error this is also the smallest I can get the change down to. The silver lining is future PRs will have much less collateral

Copy link
Contributor

@ouchadam ouchadam Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad I meant that we could format the codebase without the editconfig changes in this PR to get a baseline, then all the PRs afterwards will only contain the new changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That approach could work too, though I guess the PR for that wouldn't be this one. I'll go ahead and make a new one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closing this for now. Will reapproach after merging #5805

@ericdecanini ericdecanini requested a review from ouchadam April 19, 2022 11:27
@ericdecanini ericdecanini deleted the task/eric/parameter-new-line branch April 20, 2022 18:21
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.

Code Style Change - Move long function and class parameters to new lines
2 participants