Skip to content

Use unique type for predeclare extension declaration. #1084

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

Merged
merged 4 commits into from
Jan 12, 2022

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Jan 11, 2022

It seems because the extension is added lazily when calling predeclareDeps, Gradle does not generate Kotlin accessors to be able to just call spotlessPredeclare { } (or maybe just something I couldn't figure out well). This isn't ideal but not a big deal if we could use the other standard gradle pattern configure, but currently we cannot because there are two extensions with the same type.

configure<SpotlessPredeclareExtension> {  // Nor does SpotlessExtension work since two with that type
}

I guess there is a Gradle idiom of making sure to use unique types for extension declarations so this PR is probably needed.

Separately, I wonder if instead of having predeclareDeps in spotless should be replaced by always registering the spotlessPredeclare extension and adding a policy method or similar in it, so it is activated when calling that policy method instead of predelcareDeps on the spotless extension.

  • a summary of the change
  • either
    • a link to the issue you are resolving (for small changes)
    • a link to the PR you just created (for big changes likely to have discussion)

If your change only affects a build plugin, and not the lib, then you only need to update the plugin-foo/CHANGES.md for that plugin.

@nedtwigg
Copy link
Member

Thanks for the fix! This will definitely get merged, but I'd like to first understand your suggestion for a further change. After this PR is merged (as-is), the following will work:

configure<SpotlessExtension> {
  predeclareDeps()
  ...
}
configure<SpotlessPredeclareExtension> {
  ...
}

I wonder if instead of having predeclareDeps in spotless should be replaced by always registering the spotlessPredeclare extension ... it is activated when calling that policy method

The point of this change is to allow this:

configure<SpotlessExtension> {
  ...
}
configure<SpotlessPredeclareExtension> {
  predeclareDeps()
  ...
}

I'm okay with that change, but I don't understand what is better about it (I know zero about gradle kotlinscript). Is there something about the Gradle Kotlinscript tooling which would make this preferable? Does it let you drop the configure<> blocks?

@nedtwigg
Copy link
Member

Also we should probably add a kotlinscript test, and link to it from this part of the readme.

@nedtwigg nedtwigg enabled auto-merge January 12, 2022 22:11
@nedtwigg nedtwigg merged commit 7bd0b89 into diffplug:main Jan 12, 2022
@nedtwigg
Copy link
Member

Released in 6.2.0 because it is definitely an improvement as-is. Happy to take a future PR to implement your other suggestions if you can answer the question I raised above.

@anuraaga
Copy link
Contributor Author

Sorry for not getting to this yesterday @nedtwigg, I'm interested in adding a test for the kotlin script too.

Gradle, I guess examines extensions.create method calls invoked within Plugin.apply to generate DSL automatically. So this currently works

spotless { // Equivalent to configure<SpotlessExtension>
  java { googleJavaFormat() }
}

but this doesn't seem to work (I will confirm again on 6.2.0 in case the same issue could have caused issues with the DSL

spotlessPredeclare {  
  java { googleJavaFormat() }
}

I think having SpotlessPredeclareExtension always added within Plugin.apply instead of lazily would allow that to work (again, will verify that second syntax doesn't work on 6.2.0 in a bit

@nedtwigg
Copy link
Member

No worries! Whenever you get around to it.

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.

2 participants