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

Narrow down BuildLocalPythonDistributions target type #5659

Merged
merged 4 commits into from Apr 5, 2018

Conversation

Projects
None yet
4 participants
@UnrememberMe
Copy link
Contributor

UnrememberMe commented Apr 5, 2018

Problem

BuildLocalPythonDistribution currently works on PythonDistribution and all subclasses. This makes subclassing BuildLocalPythonDistribution difficult.

Solution

  • extract filtering code out to a function that can be overridden by subclass
  • narrow down target filtering to just PythonDistribution

Result

One can subclass PythonDistribution target and BuildLocalPythonDistribution task to add new functionality.

@kwlzn

kwlzn approved these changes Apr 5, 2018

@@ -43,8 +43,12 @@ def prepare(cls, options, round_manager):
def cache_target_dirs(self):
return True

@staticmethod
def filter_target(tgt):
return type(tgt) == PythonDistribution

This comment has been minimized.

@kwlzn

kwlzn Apr 5, 2018

Member

more idiomatic to use is vs == in this case.

@UnrememberMe

This comment has been minimized.

Copy link
Contributor

UnrememberMe commented Apr 5, 2018

  • updated per review feedback.
  • resolved master merge conflict.
@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Looks good to me. Not going to block on it but could you add a covering test case?

@stuhood stuhood merged commit e764b0c into pantsbuild:master Apr 5, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@stuhood stuhood added this to the 1.6.x milestone Apr 5, 2018

@UnrememberMe UnrememberMe deleted the UnrememberMe:UnrememberMe/subclassable_task branch Apr 9, 2018

kwlzn added a commit that referenced this pull request Apr 9, 2018

Narrow down BuildLocalPythonDistributions target type (#5659)
 ### Problem

BuildLocalPythonDistribution currently works on PythonDistribution and all subclasses.  This makes subclassing BuildLocalPythonDistribution difficult.

### Solution

 - extract filtering code out to a function that can be overridden by subclass
 - narrow down target filtering to just PythonDistribution

### Result

One can subclass PythonDistribution target and BuildLocalPythonDistribution task to add new functionality.

@kwlzn kwlzn removed the needs-cherrypick label Apr 9, 2018

@kwlzn

This comment has been minimized.

Copy link
Member

kwlzn commented Apr 9, 2018

cherry-picked to 1.6.x

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