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

Remove some Fabric shorthands #38

Draft
wants to merge 3 commits into
base: v2
Choose a base branch
from

Conversation

PaintNinja
Copy link
Member

Shorthands can be nice, but too many can introduce ambiguity and inconsistency - this PR proposes to remove some in order to help with that.

There's some other cases not included in this PR yet but may be worth considering a redesign:

  1. jar files
jars {
    jar {
        file = "nested/vendor/dependency.jar"
    }
    jar "nested/vendor/another-dependency.jar"
}

Should we add a jar(String) adder shorthand to the root? Should the shorthand be removed altogether or the jar(Closure) removed instead?

  1. mixins
mixins {
    mixin "modid.mixins.json"
    mixin {
        config = "modid-server.mixins.json"
        environment = Environment.SERVER
    }
}

Should we have closures for client, server and common inside the MixinsBuilder?

  1. icons
icon {
    x16 = "small.png"
    x32 = "medium.png"
}
icon 64, "large.png"

Should we remove the icon(int, String) adder shorthand and replace it with an icon = "medium.png" setter shorthand in the root?

@PaintNinja PaintNinja requested a review from lukebemish July 17, 2024 21:28
@@ -54,8 +54,6 @@ final mdg = FabricModsDotGroovy.make {
java = ">=17"
it.'fabric-api' = "*"

mod "another-mod", ">=1.5.0"
mod("something-else", v(">0.5") & v('<1.0'))
Copy link
Member

Choose a reason for hiding this comment

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

I would like a version range combination test like this to remain in the file still

@lukebemish
Copy link
Member

lukebemish commented Jul 18, 2024

  1. the jar { } syntax should not be removed, as this matches how we handle lists of nested blocks elsewhere and that's the current structure of that in the FMJ, and other fields besides file could presumably be added in the future. It's also how stuff is handled at the plugin level!. I'd say either keep the jar 'string' syntax or move it to the root; I'm mildly in favor of the former as it matches, say, how we treat mixin configs but I have no huge preference.
  2. I'm generally of the opinion that if you're specifying more than one field it's probably not worth the shorthand; I certainly wouldn't have closures for client and server or whatever but I suppose I can see the point of single client('string') shorthand or the like?
  3. Not sure how the proposed setter shorthand would work -- don't you need the icon size still?

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

Successfully merging this pull request may close these issues.

2 participants