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

Make PexBuilderWrapper a Subsystem. #6897

Merged
merged 1 commit into from Dec 12, 2018

Conversation

Projects
None yet
5 participants
@jsirois
Copy link
Member

jsirois commented Dec 11, 2018

This allows it to be "injected" directly into consumers, eliminating
the middleman.

@jsirois jsirois force-pushed the jsirois:pbw/law_of_demeter branch from ffe3fc2 to 125f26f Dec 11, 2018

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Looks good to me!

@stuhood
Copy link
Member

stuhood left a comment

(not a complete review)

@@ -87,7 +92,30 @@ def dump_sources(builder, tgt, log):
class PexBuilderWrapper(object):
"""Wraps PEXBuilder to provide an API that consumes targets and other BUILD file entities."""

def __init__(self, builder, python_repos_subsystem, python_setup_subsystem, log=None):
class Factory(Subsystem):

This comment has been minimized.

@stuhood

stuhood Dec 11, 2018

Member

Can we revisit whether the Factory is necessary in these cases? Is the idea behind the Factory that the Factory receives the relevant scoped Options, but that the thing the subsystem constructs might have some additional steps involved in initialization?

I've noticed that in some of those cases, using @memoized_(property|method) to memoize the additional construction steps can allow for avoiding the split between an explicit Factory/Builder and the built thing.

This comment has been minimized.

@jsirois

jsirois Dec 11, 2018

Member

Subsystem has constructor:

def __init__(self, scope, scoped_options):

I see no non-hacky way to make a class that needs more than options in its constructor be a Subsystem. Worse in this case, the additional constructor options are not global in character or even scoped in character - they are per-use. IOW there are existing use cases in this review where a task creates two different pexs using the PexBuilderWrapper - each needs its own PEXBuilder if not log. So the factory truly is a single factory built from its options (empty here - a wart of the combination of options with deps) and deps that then produces a unique PexBuilderWrapper per PEXBuilder.

This comment has been minimized.

@stuhood

stuhood Dec 11, 2018

Member

Worse in this case, the additional constructor options are not global in character or even scoped in character - they are per-use.

Ok. So in this case the factory is less about the fact that a Subsystem is involved, and more about the fact that what is being configured is not an instance of a tool, but literally a Factory/Builder that needs more information. Makes sense.

I see no non-hacky way to make a class that needs more than options in its constructor be a Subsystem.

So then, in some other hypothetical case where all the Subsystem needed was options in order to be constructed, the Factory might not be necessary? If so, that makes sense... I don't think I was making the distinction between "needs only options to be constructed" and "needs options and some other stuff" to be constructed.

This comment has been minimized.

@jsirois

jsirois Dec 11, 2018

Member

Exactly. A Subsystem, by design, can only require options and other subsystem dependencies to be constructed.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Dec 13, 2018

Contributor

This was a useful thread for me to see as I was not super clear on when to model new code after the existing .Factory classes throughout the codebase. You can see in the native backend that I have been making quite a number of subsystems and composing them, which may have led to externalizing some logic that could have been done with a factory.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Dec 11, 2018

I, for one, welcome our PexBuilderWrapperFactory overlord, in all its Enterprise Java naming glory...

@benjyw

benjyw approved these changes Dec 11, 2018

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Dec 12, 2018

Re-based against origin to pick up CI flake fixes...

Make PexBuilderWrapper a Subsystem.
This allows it to be "injected" directly into consumers, eliminating
the middleman.

@jsirois jsirois merged commit 00968cc into pantsbuild:master Dec 12, 2018

1 check passed

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

@jsirois jsirois deleted the jsirois:pbw/law_of_demeter branch Dec 12, 2018

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