Code Style Change #5644

ericdecanini · 2022-03-28T08:29:00Z

Draft until entire project is reformatted. See bottom of description for more detail

Open for discussion!

Proposal Doc

Summary:

We want to make changes to the code style for better readability and to stick closer to Kotlin code conventions.

The changes include:

Function and class parameters in new lines
Disable when branches in column
Disable all continuation indentation
Enable trailing commas
Enable place ‘)’ on new line for if statements
Disable keeping comments at first column

These changes have been decided on through chats and polls.

The changes were made on a .editorconfig file which automatically generates a project.xml file for projects that include it, which then automatically applies the code style to the IDE. This will then affect the style applied when auto formatting code.

The aforementioned .editorconfig file was generated through selecting all the necessary options in Android Studio's kotlin code style and exporting it as such. The exported file did also include style config for other languages (Java, C++, etc.) but I removed those from the config as we don't have a concensus on that yet.

~~I'm currently testing if this works too with the linter, both locally as well as through the CI~~

Edit: This affects the CI linter so the entire project needs to be reformatted. Turning this PR into a draft until that's done

github-actions · 2022-03-28T09:10:58Z

Unit Test Results

110 files ±0 110 suites ±0 1m 22s ⏱️ +9s
195 tests ±0 195 ✔️ ±0 0 💤 ±0 0 ❌ ±0
650 runs ±0 650 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 37a6e60. ± Comparison against base commit 94099f4.

♻️ This comment has been updated with latest results.

ericdecanini · 2022-03-30T13:02:25Z

Poll: "Do we want to keep arrow alignment in when statements (case -> expression)"
Majority vote: No, don't keep it (7/11 votes)

ericdecanini · 2022-03-31T13:03:51Z

Poll: "Do we want to start using trailing commas?"
Majority vote: Yes, let's start using them (5/8 votes)

ericdecanini · 2022-04-04T07:29:46Z

Poll: Do we want to placing ')' on new line for if statments?
Majority vote: Enable it (3/7)

bmarty

Some thoughts. Also it will be a nightmare to handle so many (formatting) diff for the forks with lots of changes :/. Does it worth it?

bmarty · 2022-04-07T11:26:11Z

.editorconfig

+end_of_line = crlf
+indent_size = 4
+indent_style = space
+insert_final_newline = false


it was true, I think it should stay true

Good catch!

bmarty · 2022-04-07T11:29:59Z

...ry/attachment-viewer/src/main/java/im/vector/lib/attachmentviewer/AnimatedImageViewHolder.kt

@@ -20,7 +20,7 @@ import android.view.View
 import im.vector.lib.attachmentviewer.databinding.ItemAnimatedImageAttachmentBinding

 class AnimatedImageViewHolder constructor(itemView: View) :
-        BaseViewHolder(itemView) {
+    BaseViewHolder(itemView) {


is it the "continuation indent" thing?
I do not like the new format :/

class AnimatedImageViewHolder constructor(itemView: View) : BaseViewHolder(itemView) { val views = ItemAnimatedImageAttachmentBinding.bind(itemView)

is more confusing than

class AnimatedImageViewHolder constructor(itemView: View) : BaseViewHolder(itemView) { val views = ItemAnimatedImageAttachmentBinding.bind(itemView)

if I am reading it correctly.

This is indeed. The ideal formatting here should be as follows:

class AnimatedImageViewHolder constructor( itemView: View, ) : BaseViewHolder(itemView) { val views = ItemAnimatedImageAttachmentBinding.bind(itemView)

I'll investigate if I can get the formatter to achieve this

bmarty · 2022-04-07T11:31:45Z

library/attachment-viewer/src/main/java/im/vector/lib/attachmentviewer/AttachmentsAdapter.kt

-            R.layout.item_video_attachment          -> VideoViewHolder(itemView)
-            else                                    -> UnsupportedViewHolder(itemView)
+            R.layout.item_video_attachment -> VideoViewHolder(itemView)
+            else -> UnsupportedViewHolder(itemView)


You will not convince me that the new format is easier to read. (but I know the advantages).
Applicable to nearly all the when statements.
Probably because to me code readability is top priority.

It is mine too, but personally I find this easier to read than if they're all aligned. It also may well be the case that this is easier or harder to read based on what you're used to and falls into the realm of subjective, hence, the earlier poll

bmarty · 2022-04-07T11:33:22Z

library/attachment-viewer/src/main/res/layout/activity_attachment_viewer.xml

-    android:layout_width="match_parent"
-    android:layout_height="match_parent"
-    tools:context=".AttachmentViewerActivity">
+        xmlns:tools="http://schemas.android.com/tools"


I do not understand why we now have 8 spaces for an indent.

A mistake. Good spot!

ericdecanini · 2022-04-07T12:57:21Z

Some thoughts. Also it will be a nightmare to handle so many (formatting) diff for the forks with lots of changes :/. Does it worth it?

I agree that we have to give this some proper thought before pushing through with it. For the branches of the main fork, us all autoformatting on our own branches should work but this could be a lot more involved when it comes to the forks. We might have to look at alternate approaches to this, but I do very much think it's worth it as it boosts the code's readability up a lot and opens avenues for even more improvements

bmarty · 2022-04-07T14:23:08Z

Have you considered re-enabling disabled rules? See https://github.com/vector-im/element-android/blob/main/build.gradle#L107

ericdecanini · 2022-04-11T09:10:32Z

Have you considered re-enabling disabled rules? See https://github.com/vector-im/element-android/blob/main/build.gradle#L107

Thanks for pointing this out! We'd most probably want to

ericdecanini · 2022-04-11T09:15:25Z

Closing as we decided on the approach of tackling these changes one at a time. I'll make these changes on separate branches

ericdecanini closed this Apr 6, 2022

ericdecanini force-pushed the task/eric/code-style-changes branch from 9f13d0d to 94099f4 Compare April 6, 2022 17:14

Applies code style changes to editor config

37a6e60

ericdecanini reopened this Apr 6, 2022

ericdecanini changed the title ~~Code Style Change [DISCUSSION]~~ Code Style Change Apr 7, 2022

ericdecanini requested review from a team, ouchadam and bmarty and removed request for a team April 7, 2022 08:52

ericdecanini marked this pull request as draft April 7, 2022 09:44

bmarty reviewed Apr 7, 2022

View reviewed changes

ericdecanini added 2 commits April 8, 2022 11:20

Makes fixes to editor config

624f0a3

Reformats project kotlin files

1c2b55d

ericdecanini force-pushed the task/eric/code-style-changes branch from 8efcec4 to 1c2b55d Compare April 9, 2022 09:13

ericdecanini closed this Apr 11, 2022

ericdecanini deleted the task/eric/code-style-changes branch April 11, 2022 09:15

This was referenced Apr 15, 2022

Code Style Change - Move long function and class parameters to new lines #5771

Closed

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

Closed

Formats project with new code style #5805

Merged

ericdecanini mentioned this pull request May 23, 2022

Code Style Change - Disable when arrow alignment #6126

Merged

15 tasks

ericdecanini mentioned this pull request Jul 19, 2022

Code style change: Trailing Commas #6593

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Style Change #5644

Code Style Change #5644

ericdecanini commented Mar 28, 2022 •

edited

Loading

github-actions bot commented Mar 28, 2022 •

edited

Loading

ericdecanini commented Mar 30, 2022 •

edited

Loading

ericdecanini commented Mar 31, 2022

ericdecanini commented Apr 4, 2022

bmarty left a comment

bmarty Apr 7, 2022

ericdecanini Apr 7, 2022

bmarty Apr 7, 2022

ericdecanini Apr 7, 2022

bmarty Apr 7, 2022

ericdecanini Apr 7, 2022

bmarty Apr 7, 2022

ericdecanini Apr 7, 2022

ericdecanini commented Apr 7, 2022

bmarty commented Apr 7, 2022

ericdecanini commented Apr 11, 2022

ericdecanini commented Apr 11, 2022 •

edited

Loading

Code Style Change #5644

Code Style Change #5644

Conversation

ericdecanini commented Mar 28, 2022 • edited Loading

github-actions bot commented Mar 28, 2022 • edited Loading

Unit Test Results

ericdecanini commented Mar 30, 2022 • edited Loading

ericdecanini commented Mar 31, 2022

ericdecanini commented Apr 4, 2022

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericdecanini commented Apr 7, 2022

bmarty commented Apr 7, 2022

ericdecanini commented Apr 11, 2022

ericdecanini commented Apr 11, 2022 • edited Loading

ericdecanini commented Mar 28, 2022 •

edited

Loading

github-actions bot commented Mar 28, 2022 •

edited

Loading

ericdecanini commented Mar 30, 2022 •

edited

Loading

ericdecanini commented Apr 11, 2022 •

edited

Loading