Skip to content
This repository has been archived by the owner on Jan 12, 2023. It is now read-only.

Update linters #7895

Closed
wants to merge 6 commits into from
Closed

Update linters #7895

wants to merge 6 commits into from

Conversation

rvandermeulen
Copy link
Contributor

Update ktlint to 0.47.1 and detekt to 1.21.0

@rvandermeulen
Copy link
Contributor Author

The UndocumentedPublicClass and UndocumentedPublicFunction checks caused hundreds of errors that seem pretty low-value. I guess we could just use a baseline for them otherwise, but I'd question how much we care about these in general.

I disabled UtilityClassWithPublicConstructor for now mainly just to get a green run so I could see what the rest of CI looks like afterwards. Not sure what we want to do about these - change/suppress/otherwise?

UtilityClassWithPublicConstructor - [The class AppReviewUtils only contains utility functions. Consider defining it as an object at app/src/main/java/org/mozilla/focus/appreview/AppReviewUtils.kt:23:1
UtilityClassWithPublicConstructor - [The class IconGenerator only contains utility functions. Consider defining it as an object at app/src/main/java/org/mozilla/focus/shortcut/IconGenerator.kt:20:1

@@ -73,7 +73,7 @@ open class ExceptionsListFragment : BaseSettingsLikeFragment(), CoroutineScope {
return true
}

override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This might actually need adding a dummy implementation ( maybe a single-line comment) because non-abstract methods need a body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads-up! I'm planning to come back to this PR after mozilla-mobile/reference-browser#2056 lands.

@gabrielluong
Copy link
Member

Hey @rvandermeulen , I am interested in our ktlint and detekt process so I can help take over your work to completion.

@rvandermeulen
Copy link
Contributor Author

rvandermeulen commented Dec 5, 2022

Ah, I'd been sitting on this one until the PR in r-b landed as I wanted to follow the same process for this one (and eventually Fenix/AC as well). This current patch stack didn't take the right approach compared to the one in the r-b PR.

If you want to take it, feel free though!

@rvandermeulen
Copy link
Contributor Author

I also hadn't decided whether I wanted to handle the detekt 1.22 update in that PR still vs. getting it in a follow-up (though I was sorta leaning towards the latter)

@gabrielluong
Copy link
Member

@rvandermeulen I upgraded our detekt the last time and manually compared the base configuration detekt.yml with our detekt.yml to align the changes. If it's okay with you, I can at least take over the detekt portion.

@rvandermeulen
Copy link
Contributor Author

Go for it. Probably best to start a new PR at this point.

@rvandermeulen rvandermeulen deleted the lint_update branch December 9, 2022 18:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants