Updated dependencies for v2022.1 #4883

grzesiek2010 · 2021-10-28T12:50:32Z

What has been done to verify that this works as intended?

I've run automated tests.

Why is this the best possible solution? Were any other approaches considered?

This is what we do after every release.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

It doesn't require testing.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
verified that any code or assets from external sources are properly credited in comments and/or in the about file.
verified that any new UI elements use theme colors. UI Components Style guidelines

grzesiek2010 · 2021-11-03T12:30:34Z

.editorconfig

@@ -0,0 +1,2 @@
+[*.{kt,kts}]
+disabled_rules=import-ordering


I disabled checking import ordering because it has always been kind of annoying especially that in Android Studio we don't have any easy way to reorder imports so that ktlint is satisfied. And now with new updates they changed something and I was asked to change the order of menu imports despite the fact everything was fine before.

Is there no way to fix using "Import layout" in Android Studio > Preferences > Editor > Code Style > Kotlin? It would be nice to keep ordering enforced, so we don't end up causing accidental changes in files using "optimize imports".

Ok I downgraded ktlint because they might have something broken regarding the ordering of imports in the newest version if what we have on master is no longer accepted. I even tried different settings like:

ij_kotlin_imports_layout=* # alphabetical with capital letters before lower case letters (e.g. Z before a), no blank lines ij_kotlin_imports_layout=*,java.**,javax.**,kotlin.**,^ # default IntelliJ IDEA style, same as alphabetical, but with "java", "javax", "kotlin" and alias imports in the end of the imports list

to no avail so let's check it after next release.

grzesiek2010 · 2021-11-03T12:34:00Z

collect_app/src/androidTest/java/org/odk/collect/android/feature/formentry/FormMetadataTest.kt

@@ -25,12 +25,10 @@ class FormMetadataTest {
    fun startGeopoint_withCachedFormDefinition_doesNotCauseError() {
        rule.startAtMainMenu()
            .copyForm("start-geopoint.xml")
-


They just decided we shouldn't use blank lines like that pinterest/ktlint#1248
I could disable the rule but it would also mean disabling checking blank lines in all the other cases (that I like).

I feel pretty strongly about keeping those blank lines. In a lot of case it might not make sense, but I do think it helps readability in our UI tests when related groups are broken up by a blank line like that. Can we put ignore the rule and revert related changes?

No because the rule is no-consecutive-blank-lines so as I said if you disable it you disable checking all the other cases where having blank lines is obviously a bad thing.

Sorry what I mean is that I'd be happy to just disable that rule. I feel like the blank lines are useful enough in some places to make up for other places they might appear and not look quite right.

Are you sure? I'm going to repeat: if we do that you can add multiple blank lines in many different ways in the codebase and it won't be detected. I didn't want to do that because in all the other cases detecting blank lines worked well.

Ah, you mean it's not just for chains? Sorry you're right I'm misunderstanding and that it's not worth disabling.

it's not just for chains?

yeah it's a broader rule.

You know what? now as I downgraded ktlint because of that problem with imports (see above) I can bring those lines back too. So maybe let's do that for now and complain about that in their repo (and ask if we can disable this particular check somehow if they are unwavering). What do you think?

Yeah, agreed! It does seem a little broken as a line followed by a comment shouldn't count as "consecutive" blank lines. I can post a reply (seeing as I seem to care so much 😆).

Ok I reverted that commit.

grzesiek2010 · 2021-11-03T12:35:57Z

buildSrc/src/main/java/dependencies/Dependencies.kt

@@ -97,7 +97,7 @@ object Dependencies {
    const val hamcrest = "org.hamcrest:hamcrest:2.2"
    const val powermock_module_junit4 = "org.powermock:powermock-module-junit4:2.0.9"
    const val powermock_api_mockito2 = "org.powermock:powermock-api-mockito2:2.0.9"
-    const val robolectric = "org.robolectric:robolectric:4.6.1"


I had to downgrade robolectric for now because of this issue robolectric/robolectric#6593 there is a workaround there but I don't like it so I prefer waiting for a better version of this dependency.

grzesiek2010 · 2021-11-03T12:37:24Z

build.gradle

@@ -15,7 +15,7 @@ buildscript {

    dependencies {
        classpath 'com.android.tools.build:gradle:7.0.3'
-        classpath 'com.google.gms:google-services:4.3.10'


The updated broke lint throwing hundreds Unused resource errors so I downgraded for now.

Let's pull out a separate issue for this.

I usually don't file new issues for every dependency that I need to skip during upgrading. Usually if there is something wrong next time it works fine. If during next code freeze the same issue occurs then I will do that.

This reverts commit 8fd2de5.

seadowg

Good to merge once we unfreeze!

grzesiek2010 · 2021-11-04T21:17:15Z

@seadowg

I've added (or actually removed :)) some more stuff:

ButterKnife since we should use view binding instead
findbugs since it hasn't been used for a long time either way and I think we don't need to bring it back. It checks only java code but now most of the new code is in Kotlin.

grzesiek2010 added 8 commits October 28, 2021 12:58

Reviewed and updated dependencies

75bbdbf

Disable ktlint import-ordering rule

c406116

Removed blank lines

8fd2de5

Updated checkstyle and pmd

c6471d0

Downgraded Google Services Gradle Plugin

eca1ce1

Downgraded robolctric

7e384d6

Fixed tests

d1f10c2

Removed powermock

9b9e65f

grzesiek2010 force-pushed the v2022.1_dependencies branch 2 times, most recently from 430b1e8 to 9b9e65f Compare November 3, 2021 11:56

grzesiek2010 changed the title ~~V2022.1 dependencies~~ Updated dependencies for v2022.1 Nov 3, 2021

grzesiek2010 marked this pull request as ready for review November 3, 2021 12:27

grzesiek2010 commented Nov 3, 2021

View reviewed changes

grzesiek2010 added 2 commits November 4, 2021 09:47

Downgraded ktlint and enabled checking imports

dc4629d

Revert "Removed blank lines"

8a176e1

This reverts commit 8fd2de5.

seadowg approved these changes Nov 4, 2021

View reviewed changes

grzesiek2010 added 10 commits November 4, 2021 16:03

use ViewBinding instead of ButterKnife in NoButtonsItem

083749a

use ViewBinding instead of ButterKnife in ShowQRCodeFragment

b993e12

Removed unused InstanceUploaderProgressBar

d20d9e4

use ViewBinding instead of ButterKnife in InstanceUploaderListActivity

847f86e

Don't use ButterKnife in InstanceUploaderAdapter

f0e4b90

use ViewBinding instead of ButterKnife in AudioVideoImageTextLabel

49891d1

Removed ButterKnife

8ead4ab

Removed dependencies that we no longer use from Licenses page

31398ff

Removed findbugs

1a55af4

Simplify adding layouts in custom views

a457cec

grzesiek2010 force-pushed the v2022.1_dependencies branch from 7be5fc6 to a457cec Compare November 4, 2021 21:02

grzesiek2010 requested a review from seadowg November 5, 2021 08:12

grzesiek2010 added this to the v2022.1 milestone Nov 18, 2021

seadowg approved these changes Nov 22, 2021

View reviewed changes

grzesiek2010 merged commit 1b310ce into getodk:master Nov 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated dependencies for v2022.1 #4883

Updated dependencies for v2022.1 #4883

grzesiek2010 commented Oct 28, 2021 •

edited

Loading

grzesiek2010 Nov 3, 2021

seadowg Nov 3, 2021

grzesiek2010 Nov 4, 2021 •

edited

Loading

grzesiek2010 Nov 3, 2021

seadowg Nov 3, 2021

grzesiek2010 Nov 3, 2021

seadowg Nov 4, 2021

grzesiek2010 Nov 4, 2021

seadowg Nov 4, 2021

grzesiek2010 Nov 4, 2021 •

edited

Loading

seadowg Nov 4, 2021

grzesiek2010 Nov 4, 2021

grzesiek2010 Nov 3, 2021

grzesiek2010 Nov 3, 2021

seadowg Nov 3, 2021

grzesiek2010 Nov 3, 2021

seadowg left a comment

grzesiek2010 commented Nov 4, 2021

		@@ -0,0 +1,2 @@
		[*.{kt,kts}]
		disabled_rules=import-ordering

Updated dependencies for v2022.1 #4883

Updated dependencies for v2022.1 #4883

Conversation

grzesiek2010 commented Oct 28, 2021 • edited Loading

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzesiek2010 Nov 4, 2021 • edited Loading

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

grzesiek2010 Nov 4, 2021 • edited Loading

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

seadowg left a comment

Choose a reason for hiding this comment

grzesiek2010 commented Nov 4, 2021

grzesiek2010 commented Oct 28, 2021 •

edited

Loading

grzesiek2010 Nov 4, 2021 •

edited

Loading

grzesiek2010 Nov 4, 2021 •

edited

Loading