Improve the init buildgen code resolving some TODOs
#5866
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: ShreckYe <[email protected]>
… object declaration, add overrides Co-authored-by: ShreckYe <[email protected]>
…make the name clearer
…tps://github.com/ShreckYe/mill into copilot/fix-9475931d-2457-49c4-9a71-fd7144c9d4ef
…`java` plugin applied in Gradle too` and replace some Yoda conditions
…71-fd7144c9d4ef Convert generic type parameters to abstract type members in BuildGenBase.scala
Achieved by replacing `null ([=!]=) (\w+)` with `$2 $1 null` using regex.
Prompt: ```text Find and replace all Yoda conditions using regex. ```
…g' into improve-init-guildgen
…nto improve-init-guildgen
…-members' into improve-init-guildgen
…odo' into improve-init-guildgen
…of `IrBaseInfo` with `Option[IrBaseInfo]` The original name `IrTrait` looks too broad.
…ldGenMain` and refactor `getBaseInfo` to retrieve `buildExport.projects` directly
lefou
reviewed
Sep 16, 2025
Comment on lines
8
to
11
| type M | ||
| type D | ||
| type I | ||
| type C |
There was a problem hiding this comment.
Can we have more descriptive names here?
There was a problem hiding this comment.
Yeah, they are respectively Module, Dependency, Input, and Config.
There was a problem hiding this comment.
I also just noticed that D/Dependency is no longer used in the current implementation. Shall I remove it or just keep it for possible use and better readability?
There was a problem hiding this comment.
I have removed D and renamed M and I. C is kept as renaming it would conflict with the Config nested classes. I guess it can be renamed to something like ConfigT if needed, but I am not sure whether this is conventional in Scala.
lefou
reviewed
Sep 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Consolidated follow-ups of #4586.
A summary of changes:
rename
ConfigtoMavenAndGradleCommonConfiginBuildGenUtilto make it clearerrename
optionalinBuildGenUtiltorenderIfArgsNonEmptyto make it more consistent with the other function namesremove
moduleOptionTreeinBuildGenBase.convertwhich seems redundantconvert generic type parameters to abstract type members for
BuildGenBaseand its subtypesAlso see the TODO comment which is resolved and removed and https://github.com/com-lihaoyi/mill/pull/4586/files#r1970795027. As these types
M,D, andIare only used in the trait and its subtypes, I think parameterization is not needed here, so replacing these type parameters with type members make them more readable and also consistent with the type memberC.remove some TODOs of slight significance without resolving them and convert some into ordinary comments
replace Yoda conditions
rename
IrBuildtoIrModuleBuildRename
IrTraittoIrBaseInfoand replace the original references ofIrBaseInfowithOption[IrBaseInfo]The original name
IrTraitseems too broad and is not very readable.rename
extractScopedDepstoextractConfigurationDepsinGradleBuildGenMainChange
Ito(BuildExport, Tree[Node[Option[Project]]])inSbtBuildGenMainand refactorgetBaseInfoto retrievebuildExport.projectsdirectlySee the resolved and removed TODOs for more detailed explanations of the changes. Also see the added TODO comments showing some alternative renaming options and remove them before merging.