feat(mobile): enabled DCM #17957

atollk · 2025-04-29T06:28:04Z

Description

Enables DCM (dart code metrics) for more extensive static analysis.
Most lints were already satisfied. A few others I have fixed and the rest I have disabled for now.

@shenlong-tanwen Since you mentioned that the current list of lints is still being evaluated, are there any I should remove entirely for now? For some that I tried to fix the violations for, I wasn't sure if that would actually be an improvement.

How Has This Been Tested?

Manual local run of DCM (dcm analyze lib)
GitHub CI (can't be tested yet because of missing secrets)

Checklist:

I have performed a self-review of my own code
I have made corresponding changes to the documentation if applicable
I have no unrelated changes in the PR.
I have confirmed that any new dependencies are strictly necessary.
I have written tests for new code (if applicable)
I have followed naming conventions/patterns in the surrounding code
All code in src/services/ uses repositories implementations for database calls, filesystem operations, etc.
All code in src/repositories/ is pretty basic/simple and does not have any immich specific logic (that belongs in src/services/)

shenlong-tanwen · 2025-04-30T06:39:30Z

mobile/analysis_options.yaml

@@ -23,7 +23,7 @@ linter:
  # producing the lint.

  rules:
-    # avoid_print: false  # Uncomment to disable the `avoid_print` rule
+    avoid_print: false


It is always better to use debug logs in place of print statements so having this rule in place is actually encouraged. Is there a reason on why we need to disable this?

My bad, I misread and thought it was disabled before.

shenlong-tanwen

@atollk Thanks a lot for the PR. Can you fix the failing checks?

are there any I should remove entirely for now?

What I had in mind was that we remove everything from dcm and start with few set of rules initially, fix its violations across the code base and incrementally go from there. The below is the stat on the number of violations on main

warning issues: 1485, style issues: 2449

For some that I tried to fix the violations for, I wasn't sure if that would actually be an improvement.

Can you list such rules that you feel like are not useful. We can then evaluate and remove them

shenlong-tanwen · 2025-04-30T06:40:43Z

mobile/analysis_options.yaml

@@ -23,7 +23,7 @@ linter:
  # producing the lint.

  rules:
-    # avoid_print: false  # Uncomment to disable the `avoid_print` rule
+    avoid_print: false
    # prefer_single_quotes: true # Uncomment to enable the `prefer_single_quotes` rule


We could enable the prefer_single_quotes rule in the future as well because the server uses single quotes for string and using this here would make things consistent across the codebase

mobile/lib/domain/services/sync_stream.service.dart

shenlong-tanwen · 2025-04-30T06:49:08Z

.github/workflows/static_analysis.yml

@@ -96,6 +103,11 @@ jobs:
        run: dart run custom_lint
        working-directory: ./mobile

+      - name: Run DCM
+        run: dcm analyze --ci-key="${{ secrets.DCM_CI_KEY }}" lib


Can you confirm if this works without the ci-key, because AFAIK, the open-source license do not provide us with an admin console but things should work normally without any key as long as dcm is running within the open source project to which it was allowed

Yep, it works.

.github/workflows/static_analysis.yml

shenlong-tanwen · 2025-05-07T05:56:10Z

@atollk Can you fix the failing CI and we can get this merged? I don't think we need the --ci-key and --email flags at all. Thanks a lot for working on this

atollk · 2025-05-07T15:06:45Z

@shenlong-tanwen We're still kind of stuck on the DCM issue, aren't we?
The current status is that it seems to only work on the original immich repo, not on forks, meaning no one without direct push access can properly use DCM.

Regarding ci-key and email: This is recommend by the DCM docs (https://dcm.dev/docs/ci-integrations/github-actions/#inputs-1)

shenlong-tanwen · 2025-05-08T04:47:54Z

The current status is that it seems to only work on the original immich repo, not on forks

Sadly, yes. We can move this to a draft and wait for input from @incendial here or on the discord server before we can move forward with the PR. I tested it with a fork of my own and can confirm the issue. The blog says that it should work for forks as well, so probably something broke with the recent changes in DCM

atollk · 2025-05-08T06:11:37Z

After testing some more, here's a proposal:
DCM's OSS detection mechanism seems to just be to send the current repositories remote URLs to a DCM server and check it against a database. Meaning, you can add a remote to the original repo (git remote add immich [email protected]:immich-app/immich.git) and that should make DCM detect it correctly. This would be a one time command that needs to be run for a local clone of a fork, so I'd consider it an acceptable setup for contributors. If there's no objection, I'll add that to the setup guide; and then, this PR can probably be merged.

.github/workflows/static_analysis.yml

atollk · 2025-05-11T08:30:25Z

@shenlong-tanwen looks good to go now imo

incendial · 2025-05-12T13:16:26Z

The current status is that it seems to only work on the original immich repo, not on forks

Sadly, yes. We can move this to a draft and wait for input from @incendial here or on the discord server before we can move forward with the PR. I tested it with a fork of my own and can confirm the issue. The blog says that it should work for forks as well, so probably something broke with the recent changes in DCM

Let me take a look, it should work for forks out of the box.

incendial · 2025-05-18T15:04:54Z

After some investigation I remember now that, yeah, adding a remote might be a necessary step as we can't fully rely on the fork info as the fork can be just renamed.

We can theoretically support non-renamed forks, but I'm not sure it's worth it.

So if having an additional setup step works for you, then this is probably the best way right now. Plus, I always though that this is still required for syncing the fork https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/configuring-a-remote-repository-for-a-fork

shenlong-tanwen

Let's ignore the prefer_single_quotes rule for now. The other changes look good to me. Thanks a lot for working on this.

mobile/analysis_options.yaml

shenlong-tanwen · 2025-06-09T03:10:04Z

@atollk Can you please rebase this on main and we can get this merged? Feel free to let me know if you'd like me to do it instead

atollk · 2025-06-09T06:11:46Z

@shenlong-tanwen Sure, I can rebase it. But does that mean, it's good to be merged now? By the nature of this change, it needs to be merged pretty much immediately after a rebase, before any other changes to the mobile directory.

atollk requested a review from bo0tzz as a code owner April 29, 2025 06:28

github-actions bot added the 📱mobile label Apr 29, 2025

shenlong-tanwen reviewed Apr 30, 2025

View reviewed changes

atollk force-pushed the enable-dcm branch from 3d21ef6 to 23f4322 Compare April 30, 2025 16:36

atollk commented Apr 30, 2025

View reviewed changes

.github/workflows/static_analysis.yml Show resolved Hide resolved

shenlong-tanwen added the changelog:enhancement label May 7, 2025

github-advanced-security bot found potential problems May 11, 2025

View reviewed changes

.github/workflows/static_analysis.yml Fixed Show fixed Hide fixed

shenlong-tanwen requested changes May 18, 2025

View reviewed changes

mobile/analysis_options.yaml Outdated Show resolved Hide resolved

atollk requested a review from shenlong-tanwen May 24, 2025 16:59

shenlong-tanwen approved these changes May 25, 2025

View reviewed changes

shenlong-tanwen assigned alextran1502 May 25, 2025

github-project-automation bot added this to Alex's list May 25, 2025

enable DCM in CI

1278608

atollk force-pushed the enable-dcm branch from 449f030 to 1278608 Compare June 9, 2025 08:50

alextran1502 added 4 commits June 9, 2025 10:22

Merge branch 'main' of github.com:immich-app/immich into enable-dcm

270f78c

chore: up version

0d2c8fe

chore: up version

bcfe520

chore: up version

51364fa

alextran1502 merged commit b890440 into immich-app:main Jun 9, 2025
42 checks passed

github-project-automation bot moved this to Done in Alex's list Jun 9, 2025

atollk deleted the enable-dcm branch June 10, 2025 14:09

Uh oh!

feat(mobile): enabled DCM #17957

feat(mobile): enabled DCM #17957

Uh oh!

Conversation

atollk commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How Has This Been Tested?

Checklist:

Uh oh!

shenlong-tanwen Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

atollk Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

shenlong-tanwen left a comment

Choose a reason for hiding this comment

Uh oh!

shenlong-tanwen Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

atollk May 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shenlong-tanwen Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

atollk Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shenlong-tanwen commented May 7, 2025

Uh oh!

atollk commented May 7, 2025

Uh oh!

shenlong-tanwen commented May 8, 2025

Uh oh!

atollk commented May 8, 2025

Uh oh!

Uh oh!

atollk commented May 11, 2025

Uh oh!

incendial commented May 12, 2025

Uh oh!

incendial commented May 18, 2025

Uh oh!

shenlong-tanwen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shenlong-tanwen commented Jun 9, 2025

Uh oh!

atollk commented Jun 9, 2025

Uh oh!

Uh oh!

Uh oh!

atollk commented Apr 29, 2025 •

edited

Loading