-
-
Notifications
You must be signed in to change notification settings - Fork 364
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
Fixes:#3550; Added hello-world Android Kotlin example using Mill #3679
Conversation
34ca718
to
de65f0e
Compare
Final Changes
Note@lihaoyi Thankyou Sir for the detailed feedback! I’ve addressed all the requested changes and resolved the comments. Please let me know if anything else needs attention. Please pardon me if I missed anything or made a mistake. Looking forward to your final review and any further suggestions! |
silly mistake forgot to update T.task to Task
def androidJar: T[PathRef] = Task { | ||
val jarFile: os.Path = T.dest / "app.jar" | ||
|
||
os.call( | ||
Seq( | ||
androidSdkModule().d8Path().path.toString, // Call D8 tool | ||
"--output", | ||
jarFile.toString, // Output JAR file | ||
"--no-desugaring" // Disable desugaring | ||
) ++ os.walk(compile().classes.path).filter(_.ext == "class").map( | ||
_.toString | ||
) // Get Kotlin class files, this calls compile task from Kotlinlib | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this? Isn't this the same as the implementation in AndroidAppModule
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sir it is same but think of situation like if we will implement this one(androidJar
) from the AndroidAppModule
then in that case our AndoridAppModule
is extending JavaModule
and in this we have compile
function for .java
extension only and we need this for .kt
this time so like we extended the KotlinModule
in AndroidAppKotlinModule
then we need to call compile
function here so that it will take function from the KotlinModule
rather than the JavaModule
This is My Hypothesis or Approach is this correct ?
please extend the Conversation Sir with your thoughts...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This def
does nothing, KotlinModule
already overrides compile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its shame for me actually it worked i tried this now 😅
no worries i will update the code just give me 5 mins
sorry for this misconception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lihaoyi Sir it worked even without AndroidSdkModule
Abstract 😅
i removed this too...
this is what lack of experience looks like (shame on me), highly inspired by your expertise thanks for guidance Sir...
Look great @himanshumahajan138 , will merge it and close out that part of the bounty |
Pull Request
Fixes: #3550
Description
Added
Kotlin Android "Hello World" Application Example
while reusingAndroidSdkModule
,KotlinModule
and creating new ModuleAndroidAppModule
with Kotlin Support and ProperDocumentation
.Related Issues
Checklist
Additional Notes
Actually, I was wondering that we reused the
AndroidSdkModule
but created a new ModuleAndroidAppModule
for Andorid Workflow but what we can do is to provideKotlin support in the Existing AndroidAppModule
and then based on user query they can use the functionality.So for this i need @lihaoyi Sir your Permission and Guidance, Please Review this PR suggest changes...