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

Merge TargetRoots subclasses #5648

Merged
merged 3 commits into from Apr 4, 2018

Conversation

Projects
None yet
4 participants
@stuhood
Copy link
Member

stuhood commented Apr 3, 2018

Problem

At one point it was necessary for performance purposes (due to #4533) to differentiate between ChangedTargetRoots (containing Address objects) and LiteralTargetRoots containing parsed specs... but with #4533 fixed, the split is no longer necessary.

Solution

Lift the Collection helper to pants.objects.Collection to allow for reuse, relocate Specs next to types that it holds, and merge the two TargetRoots subclasses.

@stuhood stuhood force-pushed the twitter:stuhood/undo-targetroots-split branch from 5ebbc47 to c359dfc Apr 3, 2018

@stuhood stuhood changed the title WIP: Unify Specs and TargetRoots Unify Specs and TargetRoots Apr 4, 2018

@stuhood stuhood requested review from kwlzn , benjyw and dotordogh Apr 4, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Apr 4, 2018

This should now be reviewable.

@stuhood stuhood removed request for benjyw , kwlzn and dotordogh Apr 4, 2018

@stuhood stuhood changed the title Unify Specs and TargetRoots WIP: Unify Specs and TargetRoots Apr 4, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Apr 4, 2018

Doh, sorry. Needs more work.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Apr 4, 2018

Confused: Should I review this now or hold off?

@stuhood stuhood force-pushed the twitter:stuhood/undo-targetroots-split branch from 6bdf296 to 11e218b Apr 4, 2018

@stuhood stuhood changed the title WIP: Unify Specs and TargetRoots Merge TargetRoots subclasses Apr 4, 2018

@stuhood stuhood requested review from jsirois , benjyw , kwlzn and dotordogh Apr 4, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Apr 4, 2018

Reviewable.

@jsirois

jsirois approved these changes Apr 4, 2018

@benjyw

benjyw approved these changes Apr 4, 2018

@kwlzn

kwlzn approved these changes Apr 4, 2018

Copy link
Member

kwlzn left a comment

lgtm

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Apr 4, 2018

Going to ignore the failing OSX shard and merge.

@stuhood stuhood merged commit 3ae1a08 into pantsbuild:master Apr 4, 2018

1 check failed

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

@stuhood stuhood deleted the twitter:stuhood/undo-targetroots-split branch Apr 4, 2018

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