dependency-guard icon indicating copy to clipboard operation
dependency-guard copied to clipboard

Published 20 hours ago • verified publisher icon dropbox

Configuration cache

Open qwert2603 opened this issue 2 years ago • 8 comments

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

qwert2603 avatar Sep 15 '22 12:09 qwert2603

@handstandsam hello! Can you review this PR please?

qwert2603 avatar Sep 22 '22 18:09 qwert2603

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

handstandsam avatar Sep 23 '22 01:09 handstandsam

@handstandsam are there any updates on this PR?

qwert2603 avatar Sep 27 '22 12:09 qwert2603

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 avatar Sep 27 '22 12:09 handstandsam

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

handstandsam avatar Oct 10 '22 13:10 handstandsam

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.

handstandsam avatar Oct 10 '22 14:10 handstandsam

@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 avatar Oct 10 '22 18:10 qwert2603

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

qwert2603 avatar Oct 13 '22 13:10 qwert2603

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

qwert2603 avatar Dec 12 '22 10:12 qwert2603

@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!

handstandsam avatar Dec 12 '22 21:12 handstandsam