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

Add Android Linting Example #3931

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

c0d33ngr
Copy link
Contributor

This is meant to to be a draft of item number 4 of issue #3868. The android linting

Hi @lihaoyi is this close to what you have in mind about the android linting or there's more?

@lihaoyi
Copy link
Member

lihaoyi commented Nov 10, 2024

@c0d33ngr looks good, need to include an example test and explanation so we can include it in the documentation

@himanshumahajan138
Copy link
Contributor

himanshumahajan138 commented Nov 10, 2024

@c0d33ngr Buddy you did just basic linting but this is not the appropriate way tbh i have tried this on my example and its saying the miss of class file but class files only come after the compilation

so @lihaoyi i request you to please wait till my new pull request(anndroid bundle support) gets merged coz in that i have updated the whole android app building process and made it professional (now we can include custom resources)

so @c0d33ngr sorry to interrupt you, but please understand...

@lihaoyi
Copy link
Member

lihaoyi commented Nov 10, 2024

@himanshumahajan138 completing bounties out of order is fine. I'll look at your PR if you wish and we can decide which one to merge first

@himanshumahajan138
Copy link
Contributor

@himanshumahajan138 completing bounties out of order is fine. I'll look at your PR if you wish and we can decide which one to merge first

Ya I Agree We can decide which one to merge first

i was saying before becoz there will be need for updated code as you can see in my pr i have updated the codes with resources

so that's the only thing i was concerned about rest we can decide this...

@c0d33ngr
Copy link
Contributor Author

mill.javalib.palantirformat.PalantirFormatModule.formatAll java.nio.file.NoSuchFileException: /workspaces/mill/bsp/test/src
    os.walk$stream$.attrs(ListOps.scala:205)
    os.walk$stream$.apply(ListOps.scala:166)
    os.walk$.apply(ListOps.scala:96)
    mill.javalib.palantirformat.PalantirFormatModule$.$anonfun$palantirArgs$2(PalantirFormatModule.scala:177)
    scala.collection.Iterator$$anon$10.nextCur(Iterator.scala:594)
    scala.collection.Iterator$$anon$10.hasNext(Iterator.scala:608)
    scala.collection.Iterator$$anon$6.hasNext(Iterator.scala:477)
    scala.collection.Iterator$$anon$6.hasNext(Iterator.scala:480)
    scala.collection.Iterator$$anon$9.hasNext(Iterator.scala:583)
    scala.collection.mutable.ListBuffer.addAll(ListBuffer.scala:145)
    scala.collection.mutable.ListBuffer.addAll(ListBuffer.scala:39)
    scala.collection.mutable.Growable.$plus$plus$eq(Growable.scala:69)
    scala.collection.mutable.Growable.$plus$plus$eq$(Growable.scala:69)
    scala.collection.mutable.AbstractBuffer.$plus$plus$eq(Buffer.scala:314)
    mill.javalib.palantirformat.PalantirFormatModule$.palantirArgs(PalantirFormatModule.scala:180)
    mill.javalib.palantirformat.PalantirFormatModule$.palantirAction(PalantirFormatModule.scala:131)
    mill.javalib.palantirformat.PalantirFormatModule$.$anonfun$formatAll$2(PalantirFormatModule.scala:102)
    scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)

@lihaoyi this command below produce the error output above. What could be wrong?
I'm trying to lint java files
> ./mill mill.javalib.palantirformat.PalantirFormatModule/

@c0d33ngr
Copy link
Contributor Author

@lihaoyi @0xnm does the PR addresses the android linting example opened issue? What changes are needed if it doesn't?

@lihaoyi
Copy link
Member

lihaoyi commented Nov 10, 2024

@c0d33ngr the lint error should be fixed in latest main branch, if you merge in main it should work.

Give me some time to do a proper review, I'm not familiar with android haha

@c0d33ngr c0d33ngr changed the title [WIP] Android Linting Example Add Android Linting Example Nov 10, 2024
@c0d33ngr c0d33ngr marked this pull request as ready for review November 10, 2024 22:17
@lihaoyi
Copy link
Member

lihaoyi commented Nov 13, 2024

FYI I'm planning on landing #3935 first before coming back to this, since that one is a larger and more invasive PR so this one would be easier to rebase/merge after

* specifying lint rules, managing reports, and more.
*/
@mill.api.experimental
trait AndroidLintModule extends AndroidSdkModule with JavaModule {
Copy link
Member

@lihaoyi lihaoyi Nov 13, 2024

Choose a reason for hiding this comment

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

Why are we inheriting from both AndroidSdkModule and JavaModule? Those are typically not mixed in together, so there's something weird here. It should be on one or the other but not both, and from the usage example I'm guessing it should instead inherit from AndroidAppModule

/** Usage

> ./mill show app.androidLint # Display full path to the linting report in HTML
".../out/app/androidLint.dest/report.html"
Copy link
Member

@lihaoyi lihaoyi Nov 13, 2024

Choose a reason for hiding this comment

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

We should use cat on the HTML report to ensure certain lint errors get picked up and properly reported, and discuss how the code in MainActivity.java results in the lint warnings reported


// Actual android application
object app extends AndroidLintModule {
def androidSdkModule = mill.define.ModuleRef(androidSdkModule0)
Copy link
Member

Choose a reason for hiding this comment

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

Let's include an example custom lint configuration here so people know what it looks like

/**
* Path to the project for which lint should run.
*/
def projectPath: T[PathRef] = Task.Source { millSourcePath }
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? Or can we just use millSourcePath everywhere instead?

/**
* Specifies the file format of lint report.
*/
def lintReportFormat: T[String] = Task { "html" }
Copy link
Member

Choose a reason for hiding this comment

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

The comment should list the possible formats we are aware of

/**
* Additional options for lint (e.g., enabling/disabling specific checks).
*/
def lintOptions: T[Seq[String]] = Task { Seq("--check", "NewApi,InlinedApi") }
Copy link
Member

Choose a reason for hiding this comment

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

All tasks in this module should be prefixed by androidLint to make it clear where they come from and avoid collisions with tasks coming from other traits that get mixed in

@lihaoyi
Copy link
Member

lihaoyi commented Nov 13, 2024

Took a review pass. Looks like a straightforward change but left a few nits

@c0d33ngr
Copy link
Contributor Author

Took a review pass. Looks like a straightforward change but left a few nits

I'll do the necessary changes and also include more features to it. I had to rewrite it as a module after looking through java linting in the codebase and also other android related modules

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants