Conversation
|
Can |
📝 WalkthroughWalkthroughThis pull request modernizes the project's build infrastructure and mixin configuration by integrating the Fletching Table plugin for enhanced access widener management, migrating mixin configurations from JSON to JSON5 format, and implementing dynamic access transformer generation instead of static file handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@fabric/build.gradle.kts`:
- Around line 103-123: The mixin registration is referencing the source .json5
files but Fletching Table expects converted .json files; update the mixin
registration inside the fletchingTable block (the mixins.register(...) call) to
reference the converted filenames by changing mixin("default",
"jsmacrosce-common.mixins.json5") to mixin("default",
"jsmacrosce-common.mixins.json") and mixin("fabric",
"jsmacrosce-fabric.mixins.json5") to mixin("fabric",
"jsmacrosce-fabric.mixins.json"); leave the j52j.register(...) lines as-is since
that extension performs the .json5→.json conversion.
🧹 Nitpick comments (3)
buildSrc/src/main/kotlin/multiloader-loader.gradle.kts (1)
2-2: Consider visibility for duplicate resources.
DuplicatesStrategy.EXCLUDEcan silently drop colliding resources. If overrides are expected, consider warning or explicitly documenting precedence.Possible adjustment if you want visibility on duplicates
- duplicatesStrategy = DuplicatesStrategy.EXCLUDE + duplicatesStrategy = DuplicatesStrategy.WARNAlso applies to: 55-62
common/src/main/java/com/jsmacrosce/jsmacros/client/mixin/JSMacrosMixinPlugin.java (2)
10-10: If subclassing isn’t needed, keep the plugin class final.
Removingfinalbroadens the API surface; please confirm subclassing is intended.
16-17: Nullability contract is unclear after removing@Nullable.
These methods still returnnull; consider restoring your previous@Nullableannotations (or an explicit comment) to align withIMixinConfigPluginexpectations and avoid static analysis/Kotlin interop confusion. Returning non-null values here would change mixin selection behavior.Also applies to: 30-31
Implement fletching table for automatic conversion or access wideners, jsonc files, and improve mixin management.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.