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

Optional snapshot root directory gradle property #517

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

brenpearson
Copy link

@brenpearson brenpearson commented Jul 28, 2022

Allows the consumer of the Paparazzi library to specify where they want the snapshots to be saved. This includes creating a gradle plugin extension for Paparazzi with this property in it, we can add other configurable properties to this extension in future.

This can be set as shown in the consumer's gradle file

paparazzi {
    snapshotRootDirectory = file("where/I/want/the/snapshots")
}

@brenpearson brenpearson force-pushed the brenpearson/snapshot-directory-gradle-prop branch from 8d6c2b9 to 8a8d5ae Compare July 31, 2022 13:10
val snapshotOutputDir =
extension.snapshotRootDirectory.orNull
?: project.layout.projectDirectory.dir("src/test/snapshots").asFile
snapshotOutputDir.mkdirs()
Copy link
Author

Choose a reason for hiding this comment

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

Seems like in PrepareResourcesTask to add this as an InputDirectory the directory has to exist, so in that case I have to add this to create the snapshot directory here instead of waiting until the task to take/verify the snapshots. Thoughts on if this is fine?

Copy link
Contributor

@TWiStErRob TWiStErRob Feb 28, 2023

Choose a reason for hiding this comment

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

you went from a Provider to a Provider that's resolved in configuration phase. This is happening too early here I think. I think this would be something like:

val snapshotOutputDir = extension.snapshotRootDirectory.orElse(project.layout.projectDirectory.dir("src/test/snapshots"))

and maybe that solves your need to create the dir too.

Copy link
Contributor

Choose a reason for hiding this comment

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

but it might be even better if the default was set up with snapshotRootDirectory.convention when the extension is registered:

val extension = project.extensions.create("paparazzi", PaparazziPluginExtension::class.java)
extension.snapshotRootDirectory.convention(project.layout.projectDirectory.dir("src/test/snapshots"))

@brenpearson brenpearson force-pushed the brenpearson/snapshot-directory-gradle-prop branch from e099fe2 to b4877c8 Compare August 7, 2022 00:29
@brenpearson brenpearson force-pushed the brenpearson/snapshot-directory-gradle-prop branch from b4877c8 to 8452de1 Compare August 7, 2022 00:32
@brenpearson brenpearson force-pushed the brenpearson/snapshot-directory-gradle-prop branch from 8452de1 to ecee598 Compare August 7, 2022 00:42
@JavierSegoviaCordoba
Copy link

Is there any blocker for this PR?

import java.io.File

interface PaparazziPluginExtension {
val snapshotRootDirectory: Property<File?>
Copy link
Contributor

@TWiStErRob TWiStErRob Feb 28, 2023

Choose a reason for hiding this comment

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

try to use DirectoryProperty

Choose a reason for hiding this comment

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

In Sqldelight I moved to ConfigurableFileCollection as a recommendation from @hfhbd to emulate how srcDirs works on Java and Kotlin Gradle plugins.

Copy link
Contributor

Choose a reason for hiding this comment

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

But this is a single output directory, srcDirs are multiple inputs, feels very different.

@JavierSegoviaCordoba
Copy link

@brenpearson do you still have interest in this?

I can fork your implementation if you lack of time to continue working on this.

@brenpearson
Copy link
Author

@JavierSegoviaCordoba I'm not going to be able to update this within the next couple of weeks. If you have time then by all means, otherwise I'll come back and take a look in probably 2 weeks time 🙏

@JavierSegoviaCordoba
Copy link

JavierSegoviaCordoba commented Mar 15, 2023

Can I push on this branch or should I create a new PR? @brenpearson commits are preserved in both cases.

EDIT: As there are conflicts, I am going to create a different PR and I will share the link here.

@JavierSegoviaCordoba
Copy link

Updated PR with @TWiStErRob suggestion: #736

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