1891723 - Consider Adding Fenix and AC Linters to Mach Lint and Renaming Existing Linters

Status: NEW

(Firefox for Android :: Tooling, enhancement)

Description

[Olivia Hall [:olivia]]

Using the command ./mach lint --list, you can see all of the linters available in MC.

Several of these linters prefixed by android were historically GeckoView only linters:

android-api-lint
android-checkstyle
android-format
android-javadoc
android-lint
android-test

This bug is to consider renaming these to be prefixed by geckoview and/or repurpose to include AC/Fenix.

Right now to run the core AC/Fenix lints, the developer must CD into .../mobile/android/fenix or .../mobile/android/android-components and run ./gradlew ktlintFormat and ./gradlew detekt.

Using ./mach lint will help developers familiar with MC, but not Android, to contribute and standardize linters.

Comment 1

[Olivia Hall [:olivia]]

(Tagging to monorepo-enhancements for triage, please move if out of scope.)

Comment 2

[Olivia Hall [:olivia]]

Additionally, I don't know if this should be filed separately or not, but lint failures in AC/Fenix don't show up in Phabricator automatically. (For example, like in this GeckoView Patch, reviewbot catches lint failures. However, this patch did not, except through manually noticing in try.

Edit: Filed this as bug 1892731!

Comment 3

[Olivia Hall [:olivia]]

Right now to run the core AC/Fenix lints, the developer must CD into .../mobile/android/fenix or .../mobile/android/android-components and run ./gradlew ktlintFormat and ./gradlew detekt.

Additionally, I'd like to mention, it is easy to run in the fenix directory and forget or not know to run the other, for example, android-components , when there are both fenix and android-components changes and think the project was globally linted, and not just that directory.

Comment 4

[Mark Banner (:standard8)]

This may be related/duplicate of bug 1826800, though I'm not sure.

The current android lints are defined here: https://searchfox.org/mozilla-central/source/tools/lint

Hence I suspect it would require an extension of those.

Comment 5

[Olivia Hall [:olivia]]

Filed bug 1907129 as a step for working on lints and helping scope this ticket down.

Comment 6

[Aaditya Dhingra [:adhingra]]

Attachment: Bug 1891723 - Adding `gradlew lint` to `android-{fenix|focus|ac}` linters

Comment 7

[:gerard-majax]

Can we also have --fix work, maybe in a follow up?

Comment 8

[:gerard-majax]

(In reply to :gerard-majax from comment #7)

Can we also have --fix work, maybe in a follow up?

Like i have some errors here that should be probably fixed automatically (reformatting of the code, like we do in other places):

BUILD FAILED in 29s
30 actionable tasks: 10 executed, 20 up-to-date
/home/alexandre/Documents/codaz/Mozilla/MiscWork/mozilla-source/mozilla-unified/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/HomeActivity.kt
  486:62  error  Tab character is in use.  detekt.NoTabs (android-fenix)

/home/alexandre/Documents/codaz/Mozilla/MiscWork/mozilla-source/mozilla-unified/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/crashes/UnsubmittedCrashDialog.kt
  46:1   error  Line detected, which is longer than the defined maximum line length in the code style.  detekt.MaxLineLength (android-fenix)
  46:7   error  Documentation of UnsubmittedCrashDialog is outdated                                     detekt.OutdatedDocumentation (android-fenix)
  73:13  error  The function CrashCard is too long (85). The maximum length is 75.                      detekt.LongMethod (android-fenix)

/home/alexandre/Documents/codaz/Mozilla/MiscWork/mozilla-source/mozilla-unified/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/gecko/GeckoProvider.kt
  111:1  error  Line detected, which is longer than the defined maximum line length in the code style.  detekt.MaxLineLength (android-fenix)

Especially given those too long line mentions, i dumbly splitted then in the middle and the next pass of mach lint --linter android-fenix --fix --outgoing bookmarks/central properly reformatted them to comply

Comment 9

[Polly [:polly]]

NB comment 8 is covered in https://bugzilla.mozilla.org/show_bug.cgi?id=1936391

Comment 10

[Phabricator Automation]

Comment on attachment 9428416 [details]
Bug 1891723 - Adding gradlew lint to android-{fenix|focus|ac} linters

Revision D224276 was moved to bug 1956870. Setting attachment 9428416 [details] to obsolete.