Skip to content

Conversation

@jdaugherty
Copy link
Contributor

I was speaking to https://github.com/vampire in the Gradle slack and he had some feedback on my original tck implementation. I think I implemented them the way he suggested so they're lazy and the task does not occur at configuration time.

Some highlights of that conversation:

  1. configurations are just file collections, so you can take advantage of this.
  2. If only one dependency is exported, then singleFile is a way to avoid looping over files / hard coding a name check.
  3. The original code was doing configuration resolution at configuration time which has known drawbacks.
  4. It's better to use an artifact transform or instead of using tasks like Copy/Sync, use copy { ... } or sync { ... } in a doFirst or doLast block.
  5. resolve() is 99.87% of the time the wrong solution.
  6. Copy or copy { ... } should almost 98.7% of the time be Sync or sync { ... } to not risk leaving stale files lying around.

@matrei with this new found education from an amazing developer, I thought I'd improve my original implementation =)

Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

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

Awesome!

@jdaugherty
Copy link
Contributor Author

I need to make 2 more changes, but can't until later today:

Revert the changes to the tck gradle file and change the configuration to transitive=false.

Remove the dependsOn and add an input to the configuration instead

@jdaugherty jdaugherty force-pushed the newMongoMerge branch 2 times, most recently from df8b3c4 to bc8a1ef Compare March 14, 2025 13:26
@jdaugherty
Copy link
Contributor Author

@matrei These changes are done, if the tests pass and Vampire comes back to me saying it's ok, I intend to merge. He also reminded me that dependsOn() is a smell and that it's better to set the inputs of a task. Take a look at my latest iteration.

@jdaugherty
Copy link
Contributor Author

@matrei as discussed, I'm going to go ahead and merge this. Let me know if you have any other feedback!

@jdaugherty jdaugherty merged commit 9458682 into apache:9.0.x Mar 15, 2025
18 of 19 checks passed
@jdaugherty jdaugherty deleted the newMongoMerge branch March 15, 2025 12:55
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.

3 participants