Konsist #1526

bmarty · 2023-10-10T15:37:49Z

Type of change

Feature
Bugfix
Technical: More quality on the project
Other :

Content

Add Konsist to do some static analysis on the code.
Add some test and fixes the existing issues.

Motivation and context

More quality and consistency across the codebase.

Screenshots / GIFs

NA

Tests

Konsist tests are unit test.
Smoke test for regression, but should be covered by screenshot test.

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-x-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR

… and fix existing issue

…ix existing issues

…med 'createPresenterName', and fix existing issues

ElementBot · 2023-10-10T15:39:32Z

	Fails
🚫	Danger failed to run `./tools/danger/dangerfile-lint.js`.

Error Error

Cannot find module 'danger-plugin-lint-report'
Require stack:
- ./tools/danger/dangerfile-lint.js
- /usr/src/danger/dist/runner/runners/inline.js
- /usr/src/danger/dist/commands/danger-runner.js
Error: Cannot find module 'danger-plugin-lint-report'
Require stack:
- ./tools/danger/dangerfile-lint.js
- /usr/src/danger/dist/runner/runners/inline.js
- /usr/src/danger/dist/commands/danger-runner.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:931:15)
    at Function.Module._load (internal/modules/cjs/loader.js:774:27)
    at Function._module2.default._load (/usr/src/danger/node_modules/override-require/dist/overrideRequire.js:43:25)
    at Module.require (internal/modules/cjs/loader.js:1003:19)
    at require (internal/modules/cjs/helpers.js:107:18)
    at Object.<anonymous> (./tools/danger/dangerfile-lint.js:9:18)
    at Module._compile (internal/modules/cjs/loader.js:1114:14)
    at requireFromString (/usr/src/danger/node_modules/require-from-string/index.js:28:4)
    at /usr/src/danger/dist/runner/runners/inline.js:161:68
    at step (/usr/src/danger/dist/runner/runners/inline.js:52:23)

Dangerfile

------------------^

Generated by 🚫 dangerJS against 7c5a41f

github-actions · 2023-10-10T16:15:07Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/k9z6CV

…ix new detected issues.

sonarqubecloud · 2023-10-10T20:32:15Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

codecov · 2023-10-10T20:44:54Z

Codecov Report

Attention: 118 lines in your changes are missing coverage. Please review.

Comparison is base (6e9d198) 58.96% compared to head (7c5a41f) 59.03%.
Report is 8 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1526      +/-   ##
===========================================
+ Coverage    58.96%   59.03%   +0.06%     
===========================================
  Files         1137     1152      +15     
  Lines        30324    30360      +36     
  Branches      6223     6220       -3     
===========================================
+ Hits         17882    17923      +41     
+ Misses        9764     9759       -5     
  Partials      2678     2678

Files	Coverage Δ
...c/main/kotlin/io/element/android/x/MainActivity.kt	`0.00% <ø> (ø)`
...c/main/kotlin/io/element/android/x/di/AppModule.kt	`0.00% <ø> (ø)`
...o/element/android/appnav/LoggedInEventProcessor.kt	`0.00% <ø> (ø)`
...eatures/createroom/impl/addpeople/AddPeopleView.kt	`45.23% <ø> (ø)`
...createroom/impl/configureroom/ConfigureRoomView.kt	`56.39% <ø> (ø)`
...eatures/createroom/impl/root/CreateRoomRootView.kt	`58.69% <ø> (ø)`
...ndroid/features/invitelist/impl/InviteListState.kt	`100.00% <100.00%> (+12.50%)`	⬆️
...android/features/invitelist/impl/InviteListView.kt	`60.25% <ø> (ø)`
...res/invitelist/impl/components/InviteSummaryRow.kt	`86.02% <ø> (ø)`
...t/android/features/leaveroom/api/LeaveRoomState.kt	`100.00% <100.00%> (+7.69%)`	⬆️
... and 104 more

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

julioromano

Just some comments

julioromano · 2023-10-11T12:49:39Z

app/src/test/kotlin/io/element/android/app/KonsistTest.kt

+    }
+
+    @Test
+    fun `Top level function with '@Composable' annotation starting with a upper case should be placed in a file with the same name`() {


Isn't detekt already checking this (or maybe it checks something similar?)?

No, Detekt is checking class name / filename, but not Composable function name / filename

julioromano · 2023-10-11T13:02:36Z

app/src/test/kotlin/io/element/android/app/KonsistTest.kt

+    }
+
+    @Test
+    fun `Data class state MUST not have default value`() {


Should we at least allow a default {} for the event sink?

This idea is to help the developer remember (by not compiling the code otherwise) that all the fields in the state should be provided when created by the presenter. So I think it's safer to enforce this for all the params.

julioromano · 2023-10-11T13:03:11Z

app/src/test/kotlin/io/element/android/app/KonsistTest.kt

+    }
+
+    @Test
+    fun `no field should have 'm' prefix`() {


julioromano · 2023-10-11T14:10:52Z

...m/src/main/kotlin/io/element/android/libraries/designsystem/components/BlurHashAsyncImage.kt

@@ -71,7 +71,7 @@ fun BlurHashAsyncImage(
 }

 @Composable
-fun BlurHashImage(
+private fun BlurHashImage(


Are these visibility changes enforced by konsist too?

Yes, else the file should be renamed to BlurHashImage.kt, which mean that actually this function should be moved to its own file).

The inner rule (of mine) is: one public composable per file.

bmarty · 2023-10-11T14:40:15Z

Thanks for the the review @julioromano
CI failure does not seem to be related to the content of this PR, let's merge it 🤞

bmarty added 7 commits October 10, 2023 14:21

Add some Konsist test.

6e5caeb

Konsist: add test to ensure filename match top level Composable View,…

8929a90

… and fix existing issue

Konsist: add test Data class state MUST not have default value, and f…

d55df22

…ix existing issues

Konsist: add test Function which creates Presenter in test MUST be na…

0bc2020

…med 'createPresenterName', and fix existing issues

data object

97fea4e

Rename some test

001bc21

Add small doc about Konsist

54f9835

bmarty requested a review from a team as a code owner October 10, 2023 15:37

bmarty requested review from jmartinesp and removed request for a team October 10, 2023 15:37

Changelog

03fb2b0

Update screenshots

837d40f

bmarty added 2 commits October 10, 2023 21:42

Konsist: improve test about no default value for State classes, and f…

4338d82

…ix new detected issues.

Konsist: no field should have 'm' prefix, and fix new detected issues.

7c5a41f

bmarty requested review from a team and julioromano and removed request for jmartinesp and a team October 11, 2023 08:51

julioromano reviewed Oct 11, 2023

View reviewed changes

julioromano approved these changes Oct 11, 2023

View reviewed changes

bmarty merged commit 6a28814 into develop Oct 11, 2023

bmarty deleted the feature/bma/konsist branch October 11, 2023 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Konsist #1526

Konsist #1526

Uh oh!

bmarty commented Oct 10, 2023

Uh oh!

ElementBot commented Oct 10, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Oct 10, 2023

Uh oh!

sonarqubecloud bot commented Oct 10, 2023

Uh oh!

codecov bot commented Oct 10, 2023

Uh oh!

julioromano left a comment

Uh oh!

julioromano Oct 11, 2023

Uh oh!

bmarty Oct 11, 2023

Uh oh!

julioromano Oct 11, 2023

Uh oh!

bmarty Oct 11, 2023

Uh oh!

julioromano Oct 11, 2023

Uh oh!

julioromano Oct 11, 2023

Uh oh!

bmarty Oct 11, 2023

Uh oh!

bmarty commented Oct 11, 2023

Uh oh!

Uh oh!

Konsist #1526

Konsist #1526

Uh oh!

Conversation

bmarty commented Oct 10, 2023

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Uh oh!

ElementBot commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Error Error

Dangerfile

Uh oh!

github-actions bot commented Oct 10, 2023

Uh oh!

sonarqubecloud bot commented Oct 10, 2023

Uh oh!

codecov bot commented Oct 10, 2023

Codecov Report

Uh oh!

julioromano left a comment

Choose a reason for hiding this comment

Uh oh!

julioromano Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

bmarty Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

julioromano Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

bmarty Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

julioromano Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

julioromano Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

bmarty Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

bmarty commented Oct 11, 2023

Uh oh!

Uh oh!

ElementBot commented Oct 10, 2023 •

edited

Loading