search

dropbox / dependency-guard

A Gradle plugin that guards against unintentional dependency changes.
Apache License 2.0
406 stars 15 forks source link

Configuration cache #51

Closed qwert2603 closed 1 year ago

qwert2603 commented 1 year ago

Fixes #28 and improves test coverage. Changes are split to commits to simplify review.

qwert2603 commented 1 year ago

@handstandsam hello! Can you review this PR please?

handstandsam commented 1 year ago

Ahh thanks, didn't see this one. Thanks for tagging, will review in the morning. Thanks!

qwert2603 commented 1 year ago

@handstandsam are there any updates on this PR?

handstandsam commented 1 year ago

I didn't forget about this, but have been busy at work and haven't gotten a chance to pull down and review it. Sorry. Still on my radar.

handstandsam commented 1 year ago

Thanks for rebasing and updating. I might defer to @autonomousapps or @joshafeinberg as I haven't implemented configuration cache myself.

handstandsam commented 1 year ago

Thanks for all this work! Because it's such a large change we'll probably have some back and forth on this one.

I know you split things up into commits, but there are 14, so it's hard even with that. If you break this apart into smaller PRs that build on each other, that may make this easier to merge in.

I added a bunch of comments, but I still will want eyes from someone who knows Configuration Cache (as I have not done any conversions).

Additionally, I want to not use Internal APIs unless it's absolutely required like it was for :buildEnvironment.

Thanks, and sorry this took me a while to get to reviewing.

qwert2603 commented 1 year ago

@handstandsam yes, I agree that large changes make some subtle bugs harder to find. So I created PR #57 with the first part of the changes.

I added a bunch of comments,

Do you mean comments on the code? I have not seen ones..

qwert2603 commented 1 year ago

Arghh, the request for review from joshafeinberg was removed when requesting a review from handstandsam

qwert2603 commented 1 year ago

@handstandsam Hi! Are there any updates on this PR?

handstandsam commented 1 year ago

@qwert2603 - Sorry, dependency-guard has been working for our use cases so I haven't had any extra time to look into this. Configuration Cache is important, but the workaround ensures that configuration cache is aware of it must running. Therefore it is working, but not with config cache.

In order to feel comfortable merging, I would want to sit down and run the new code as it's not small changes.

I sent you a DM on Kotlin Lang Slack about this topic if you can check and reply there. Thank you!