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

Vary snapshot output dir based on applied kotlin plugin #1111

Conversation

jrodbx
Copy link
Collaborator

@jrodbx jrodbx commented Sep 28, 2023

Closes #595

@jrodbx jrodbx requested a review from JakeWharton September 28, 2023 07:22
@jrodbx
Copy link
Collaborator Author

jrodbx commented Sep 28, 2023

Closing, to think about this a little more.

Moving away from withType callbacks results in spaghetti code around determining snapshotSourceSet depending on which plugin is applied. Using hasPlugin synchronous code causes plugin ordering issues.

I think the plugin code might need some more refactoring to better enable this.

@jrodbx
Copy link
Collaborator Author

jrodbx commented Sep 28, 2023

question for @JakeWharton @TWiStErRob: should we require a kotlin plugin to be added? I think right now, we haven't required it, but it might be antiquated to support this?

@JakeWharton
Copy link
Collaborator

Not a huge fan of forcing it when it's not strictly required. There's absolutely people still writing Java that will use Paparazzi.

@TWiStErRob
Copy link
Contributor

Agreed. Support it, but don't require it.
What would be the reason to require it?

I think requiring near-latest AGP is already a higher than necessary barrier to entry, but it's a good tradeoff for maintenance and moving fast.

@autonomousapps
Copy link
Contributor

I'm still strongly in favor of using compileOnly for dependencies on other plugins. I note that this plugin has an implementation dependency in KGP; is that necessary? That'll put KGP on the build classpath even if the project using Paparazzi doesn't use Kotlin.

The other issue with having strong dependencies on other plugins is the way Gradle's classloader hierarchy works. Depending on where this plugin gets added to a build, it can complicate things for users who want to use different versions of the other plugins.

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.

4 participants