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

Consolidate Resettable instances #6604

Merged
merged 3 commits into from Oct 9, 2018

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

stuhood commented Oct 7, 2018

Problem

Given that we always reset the entire Store and CommandRunner at once, the usage of Resettable in those modules is overly fine grained.

Solution

Move the Store and CommandRunner into a single external Resettable instance, and then remove all internal usage of Resettable.

Result

Simpler code, possibly slightly better performance.

@stuhood stuhood force-pushed the twitter:stuhood/resettable-store branch from e65a7f2 to 815066d Oct 8, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Works for me :) Thanks!

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Looks good to me too!

@stuhood stuhood merged commit 883c54f into pantsbuild:master Oct 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/resettable-store branch Oct 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment