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 Kotlin examples for builds and linting #3555

Merged
merged 10 commits into from
Sep 18, 2024

Conversation

0xnm
Copy link
Contributor

@0xnm 0xnm commented Sep 15, 2024

This PR partially addresses Part 2 of #3451, it add builds and linting examples.

For the linting example I used detekt for the static code analysis (error-prone doesn't support Kotlin) and ktlint for formatting.

Note: builds/8-compat-modules used src/java structure which is dictated by the MavenModule, but ideally it should be src/kotlin for Kotlin files and maybe src/java for Java files.

@0xnm 0xnm force-pushed the nogorodnikov/add-more-kotlin-examples branch from d0396dd to 9d83320 Compare September 15, 2024 21:43
@lihaoyi
Copy link
Member

lihaoyi commented Sep 15, 2024

At a first glance the approach looks reasonable. As your TODOs mention, explicit DetektModule and KLintModule definitely seems like the way to go.

For the src/main/java thing, is it possible to move it to src/main/kotlin somehow? For Scala we do it with the distinction between MavenModule extends Java Module and SbtModule extends ScalaModule with MavenModule, maybe we can use KotlinMavenModule extends KotlinModule with MavenModule to provide the src/main/kotlin source root in addition to src/main/java to support mixed language modules (does Kotlin have those?)

@0xnm 0xnm force-pushed the nogorodnikov/add-more-kotlin-examples branch 2 times, most recently from b81cae3 to c80199b Compare September 16, 2024 20:33
@0xnm 0xnm force-pushed the nogorodnikov/add-more-kotlin-examples branch from c80199b to 6c502ba Compare September 16, 2024 20:55
@0xnm
Copy link
Contributor Author

0xnm commented Sep 16, 2024

I created KotlinMavenModule and KotlinMavenModuleTests which both support src/main/[java|kotlin] and src/test/[java|kotlin] paths respectively.

Indeed it is possible to have a mix of the java/kotlin folders, and normally .java files can be found only in java folder, but .kt files can be found in both. Nothing prevents though putting .java files in kotlin folder as well (although it is quite strange) - Kotlin compiler supports both file types.

@0xnm 0xnm marked this pull request as ready for review September 16, 2024 21:17
@lihaoyi
Copy link
Member

lihaoyi commented Sep 17, 2024

Generally looks good. The ktlint and detekt should be moved into module traits, as you noted. IMO they can just go directly into kotlinlib/src/ for now rather than into contrib/, since they seem pretty lightweight wrappers around the external classpath logic

We should also include an autoformatting integration in example/kotlinlib/linting, e.g. https://github.com/facebook/ktfmt, with the same set up ScalafmtModule (i.e. people should be able to run ./mill mill.kotlinlib.KtfmtModule/ and have it autoformat kotlin code in their project without needing to mix in any traits)

@lihaoyi
Copy link
Member

lihaoyi commented Sep 17, 2024

I just realized that ktlint has a builtin formatter; in that case maybe just expand the example to make use of that? Having multiple formatters available would be nice but to start with just having one is enough

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

I don't think all these type aliases are necessary or should be there at all. Instead, users should just include either the scalalib or even better the javalib versions. E.g if users see a MavenModule in the kotlinlib package as an addition to the version in javalib they might think it's already preconfigured to handle Kotlin sources, which isn't the case.

Instead introduce a KotlinMavenModule which is handling the Kotlin specifics, but extends from javalib.MavenModule. This will also ensure, that users who already customized MavenModule to their needs, don't get their sources overridden or changed twice.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 17, 2024

I think the type MavenModule alias is unnecessary since we have our own KotlinMavenModule here, but let's keep the other ones.

I think it's a nice property that someone writing Kotlin shouldn't generally just need kotlinlib._ and shouldn't need to dig around scalalib and javalib, just like someone writing Scala generally just needs mill._, scalalib._ and doesn't need to dig around mill.api.*, mill.define.*, etc.. Obviously this isn't strictly true, but it's true enough in the common cases that I think it's still worth trying to achieve.

@lefou
Copy link
Member

lefou commented Sep 17, 2024

I think the type MavenModule alias is unnecessary since we have our own KotlinMavenModule here, but let's keep the other ones.

I think it's a nice property that someone writing Kotlin shouldn't generally just need kotlinlib._ and shouldn't need to dig around scalalib and javalib, just like someone writing Scala generally just needs mill._, scalalib._ and doesn't need to dig around mill.api.*, mill.define.*, etc.. Obviously this isn't strictly true, but it's true enough in the common cases that I think it's still worth trying to achieve.

I disagree. Once we really move classes that are now only aliases into javalib, this story changes. E.g. SbtModule already inherits MavenModule and ScalaModule inherits JavaModule which is a designated javalib member.

In fact extending javalib traits in scalalib or kotlinlib makes Mill the great polyglot build tool it currently is. I use these properties successfully in real work projects. A isolated kotlinlib is useless, if all plugins that work for JavaModule can't be applied to mixed Java/Kotlin modules.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 17, 2024

@lefou I don't think what your saying is contradictory with mine. I agree that having the different *libs depend on each other is both great and necessary. I just would like to provide a complete-ish API to end users with a single import, which is what the aliases give us, rather than asking them to import multiple things for hello-world projects.

Not everyone agrees that having one import is necessary - e.g. lot of akka makes you import stuff from 5-10 different packages to get anything done. But most of com-lihaoyi is built around trying to reach one or even zero-import usage. It's a stylistic choice

There's definitely an arbitrary line drawn in every case, and there'll be edge cases around ambiguity and stuff, but let's keep the aliases in kotlinlib/package.scala for this initial set of kotlin PRs, and we can discuss changing the convention separately

@lefou
Copy link
Member

lefou commented Sep 17, 2024

There's definitely an arbitrary line drawn in every case, and there'll be edge cases around ambiguity and stuff, but let's keep the aliases in kotlinlib/package.scala for this initial set of kotlin PRs, and we can discuss changing the convention separately

@lihaoyi Sure. You have the final saying here. Removing stuff is harder than adding. Any alias removal is a source incompatible change.

@0xnm
Copy link
Contributor Author

0xnm commented Sep 17, 2024

I extracted detekt and ktlint checkers into a dedicated traits and added very basic args there.

Added a formatting option and example for ktlint as well.

MavenModule removed from re-exports of kotlinlib and KotlinMavenModule now inherits from MavenModule as well.

I would say that having re-exports is handy, because having import from scalalib, javalib, etc. to write a build script for a single-language codebase is kind of confusing. But indeed, it makes binary compatibility more fragile to the changes in the future.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 18, 2024

Looks good @0xnm ! Going to merge it so you can proceed with the remaining few folders

@lihaoyi lihaoyi merged commit 07e1738 into com-lihaoyi:main Sep 18, 2024
24 checks passed
@lefou lefou added this to the 0.12.0 milestone Sep 18, 2024
lihaoyi added a commit that referenced this pull request Sep 23, 2024
Apart from adding `.adoc` pages for all the kotlin examples
#3585
#3589
#3555 and the palantir java
format example from
https://github.com/com-lihaoyi/mill/pulls?page=2&q=is%3Apr+is%3Aclosed,
I moved some docs around: the first few things under `Foo Example
Builds` really belong under `Foo Module Configuration`, the
`external/libraries/` folder should be in `depth/libraries/`

The test for `Custom Main Argument Parsers` seems to have bitrotted
since it was implemented, and it accidentally wasn't running in the test
suite before so we didn't notice. For now just disabling that test and
we can fix it later
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