Refactor select interpreter #4337

Merged
merged 2 commits into from Mar 17, 2017

Conversation

Projects
None yet
2 participants
@benjyw
Contributor

benjyw commented Mar 16, 2017

Refactor the new SelectInterpreter and GatherSources tasks.

Basically just breaks up the big execute() methods
into several helpers.

This is for two reasons:

  • Makes it easier to test invalidation (see below).
  • Will enable selecting interpreters and gathering sources
    separately for each target and its deps (for use by the
    PythonEval task) side-by-side with the current functionality
    of selecting a single interpreter and creating a single
    source PEX for the entire closure. This will be implemented
    in a future change.

Note that testing invalidation isn't an idle idea - I noticed
a bug in the custom fingerprint strategy that was mixing in
the target payload for no good reason. This change fixes
that bug and adds a test for it.

@benjyw benjyw requested a review from kwlzn Mar 16, 2017

Refactor the new SelectInterpreter and GatherSources tasks.
Basically just breaks up the big execute() methods
into several helpers.

This is for two reasons:

- Makes it easier to test invalidation (see below).
- Will enable selecting interpreters and gathering sources
  separately for each target and its deps (for use by the
  PythonEval task) side-by-side with the current functionality
  of selecting a single interpreter and creating a single
  source PEX for the entire closure. This will be implemented
  in a future change.

Note that testing invalidation isn't an idle idea - I noticed
a bug in the custom fingerprint strategy that was mixing in
the target payload for no good reason.  This change fixes
that bug and adds a test for it.
@kwlzn

kwlzn approved these changes Mar 17, 2017

lgtm!

+ self.tgt4 =self._fake_target('tgt4', compatibility=['FakePython<2.99.999'])
+ self.tgt20 =self._fake_target('tgt20', dependencies=[self.tgt2])
+ self.tgt30 =self._fake_target('tgt30', dependencies=[self.tgt3])
+ self.tgt40 =self._fake_target('tgt40', dependencies=[self.tgt4])

This comment has been minimized.

@kwlzn

kwlzn Mar 17, 2017

Member

s/=self/= self/

@kwlzn

kwlzn Mar 17, 2017

Member

s/=self/= self/

This comment has been minimized.

@benjyw

benjyw Mar 17, 2017

Contributor

Ooops, fixed.

@benjyw

benjyw Mar 17, 2017

Contributor

Ooops, fixed.

@benjyw benjyw merged commit f65bc3d into pantsbuild:master Mar 17, 2017

1 check was pending

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

@benjyw benjyw deleted the benjyw:refactor_select_interpreter branch Mar 17, 2017

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Refactor the new SelectInterpreter and GatherSources tasks. (#4337)
Basically just breaks up the big execute() methods
into several helpers.

This is for two reasons:

- Makes it easier to test invalidation (see below).
- Will enable selecting interpreters and gathering sources
  separately for each target and its deps (for use by the
  PythonEval task) side-by-side with the current functionality
  of selecting a single interpreter and creating a single
  source PEX for the entire closure. This will be implemented
  in a future change.

Note that testing invalidation isn't an idle idea - I noticed
a bug in the custom fingerprint strategy that was mixing in
the target payload for no good reason.  This change fixes
that bug and adds a test for it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment