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

Remove unused dependencies #5523

Merged
merged 2 commits into from
Jan 2, 2024
Merged

Conversation

Pururun
Copy link
Contributor

@Pururun Pururun commented Nov 29, 2023


This change is Reviewable

Copy link

linear bot commented Nov 29, 2023

DROID-552 Remove unused dependencies

Use The Dependency Analysis Gradle Plugin (DAGP) to find any unused dependencies and remove them.

This part of the weekly code cleanup task.

@Pururun Pururun force-pushed the remove-unused-dependencies-droid-552 branch 2 times, most recently from 10cb440 to 4ed049d Compare November 30, 2023 07:44
@Pururun Pururun self-assigned this Nov 30, 2023
@Pururun Pururun added the Android Issues related to Android label Nov 30, 2023
@Pururun Pururun requested a review from Rawa November 30, 2023 09:23
Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 8 files at r1, all commit messages.
Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @Pururun)


android/app/build.gradle.kts line 343 at r1 (raw file):

    implementation(Dependencies.Koin.core)
    implementation(Dependencies.Koin.android)
    implementation(Dependencies.Koin.compose)

I believe this one should be kept, it is used by the new navigation PR.


android/service/build.gradle.kts line 54 at r1 (raw file):

    implementation(project(Dependencies.Mullvad.ipcLib))
    implementation(project(Dependencies.Mullvad.modelLib))
    implementation(project(Dependencies.Mullvad.resourceLib))

Since it handles the notifications shouldn't it also need the resourceLib to get its' strings?

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 8 files reviewed, 3 unresolved discussions (waiting on @Pururun)


android/buildSrc/src/main/kotlin/Dependencies.kt line 0 at r1 (raw file):
We have some weird additions in this file even though we made just removals from the dependencies, @albin-mullvad can you double check to see if the file would be the same for you?

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 8 files reviewed, 3 unresolved discussions (waiting on @Pururun)


android/service/build.gradle.kts line 54 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Since it handles the notifications shouldn't it also need the resourceLib to get its' strings?

Seems like this gets retrieved through commonLib. Should we keep it for clarify or not?

@Pururun Pururun marked this pull request as ready for review November 30, 2023 09:37
@Pururun Pururun force-pushed the remove-unused-dependencies-droid-552 branch from 4ed049d to c32c612 Compare December 1, 2023 10:35
Copy link
Contributor Author

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 8 files reviewed, 3 unresolved discussions (waiting on @Rawa)


android/app/build.gradle.kts line 343 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

I believe this one should be kept, it is used by the new navigation PR.

Done

@Pururun Pururun force-pushed the remove-unused-dependencies-droid-552 branch from c32c612 to fe40c94 Compare December 4, 2023 09:00
Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)

@Pururun Pururun force-pushed the remove-unused-dependencies-droid-552 branch from fe40c94 to b3702bd Compare December 6, 2023 22:33
@Pururun Pururun force-pushed the remove-unused-dependencies-droid-552 branch from b3702bd to 4272e91 Compare December 19, 2023 10:21
Copy link
Contributor Author

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 8 files reviewed, 1 unresolved discussion (waiting on @Rawa)


android/buildSrc/src/main/kotlin/Dependencies.kt line at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

We have some weird additions in this file even though we made just removals from the dependencies, @albin-mullvad can you double check to see if the file would be the same for you?

I managed to run the update lock file script in docker. @albin-mullvad can you verify that the output is correct?

@Pururun Pururun force-pushed the remove-unused-dependencies-droid-552 branch 2 times, most recently from 4bcfbf1 to 6926380 Compare January 2, 2024 08:28
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 8 files at r1, 1 of 5 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)


android/buildSrc/src/main/kotlin/Dependencies.kt line at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I managed to run the update lock file script in docker. @albin-mullvad can you verify that the output is correct?

I've verified that it looks correct by re-generating the lockfile locally.

@Pururun Pururun force-pushed the remove-unused-dependencies-droid-552 branch from 6926380 to bea3e19 Compare January 2, 2024 10:42
Copy link
Contributor Author

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)


android/service/build.gradle.kts line 54 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Seems like this gets retrieved through commonLib. Should we keep it for clarify or not?

I think we can keep resourceLib for clarity, what do you think @albin-mullvad ?

Copy link
Contributor Author

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


android/service/build.gradle.kts line 54 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I think we can keep resourceLib for clarity, what do you think @albin-mullvad ?

After discussion with @albin-mullvad we decided to remove it as it would make it easier to use such tools in the future.

@Pururun Pururun merged commit 5844859 into main Jan 2, 2024
20 checks passed
@Pururun Pururun deleted the remove-unused-dependencies-droid-552 branch January 2, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants