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

Output Caching: invoke output value function again, after executing output effect #169

Merged
merged 2 commits into from Jul 27, 2018

Conversation

Projects
None yet
2 participants
@bpholt
Contributor

bpholt commented Jul 26, 2018

fixes #168

This is a little earlier than I would normally create a PR, but I'm having trouble locally using the snapshot version of this library in a scripted test for the sbt plugin where this issue first came up. I'm hoping to get some feedback from maintainers (and any CI for the project) and will proceed accordingly.

@eed3si9n

LGTM

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Jul 27, 2018

Member

It would be good to add a unit test for this bad bug.

Member

eed3si9n commented Jul 27, 2018

It would be good to add a unit test for this bad bug.

@bpholt

This comment has been minimized.

Show comment
Hide comment
@bpholt

bpholt Jul 27, 2018

Contributor

I agree—I’m hoping I’ll have something to add to the PR tomorrow.

Contributor

bpholt commented Jul 27, 2018

I agree—I’m hoping I’ll have something to add to the PR tomorrow.

@bpholt

This comment has been minimized.

Show comment
Hide comment
@bpholt

bpholt Jul 27, 2018

Contributor

@eed3si9n I updated the output caching tests to catch the bug. I also went through and cleaned up a bunch of compiler warnings in the util-tracking project, but I committed those changes separately in case you want to remove them and let this PR focus on the core change.

Contributor

bpholt commented Jul 27, 2018

@eed3si9n I updated the output caching tests to catch the bug. I also went through and cleaned up a bunch of compiler warnings in the util-tracking project, but I committed those changes separately in case you want to remove them and let this PR focus on the core change.

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Jul 27, 2018

Member

Awesome. I am happy to merge this.

Member

eed3si9n commented Jul 27, 2018

Awesome. I am happy to merge this.

@eed3si9n eed3si9n merged commit 404ea50 into sbt:1.x Jul 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment