Refactor WhatChanged into base class, use LazySoruceMapper

Review Request #1534 — Created Dec. 20, 2014 and submitted

davidt
pants
884
1535
80ab6c1...
pants-reviews
benjyw, dturner-tw, jsirois, patricklaw

Switch to using LazySourceMapper to map changed files to targets.
LazySourceMapper is designed to map sources to targets without
loading any more of the BUILD graph than is required, which is
ideal for a what-changed use-case.

Refactoring the what-changed calculation into base class should make
it easier to support other tasks that require the set of changed
targets (or files). The WhatChanged task itself is now just a small
stub on top of it.

https://travis-ci.org/pantsbuild/pants/builds/44683965

./pants goal test tests/python/pants_test/tasks:what_changed

  • 0
  • 0
  • 5
  • 0
  • 5
Description From Last Updated
PA
  1. 
      
  2. I feel like this class should be a mixin that provides self.mapper as a property and the two other methods, and which the inheriting class has to explicitly call out to for option registration. It doesn't seem to do enough to justify being a proper base class, but it's definitely going to cause Task diamonds for, e.g., a JvmTask that also wants to be ChangedFileBaseTask.

    1. I believe this is usable as a mixin as-is and is used as such in class WhatChanged. Changed the name and made it extend object to force it to be a mixin, but it has to be mixed into a Task anyway (eg it uses self.context to construct LSM) so I'm not sure how much that matters. What's the advantage of forcing users to make an explicit registration call?

    2. It's okay for a mixin to require that it be mixed into an object with a certain base class. However, the mixin certainly should not extend that base class. These are not, unfortunately, scala traits.

      There isn't an advantage to forcing users to make an explicit registration call--there's just no other way. You can't guarantee the MRO for register_options in all cases, and honestly it's confusing to even try to do so. As a rule, mixins should never override any methods on the class being mixed into--they should only define new, orthogonal methods.

    3. If I can't count on the MRO being "mixins before base class", then I also can't even use an __init__ to setup attributes. I don't think I want add a bunch of @properties that go caching things in self.__dict__ just to get around having an __init__ with a call to super and extending mixins before a base class.

      I'm fine with a mixin overriding methods as long as it a) calls super and b) is inherited before base class, but if that isn't something I can assume, then I think my preference is that this just isn't a mixin, and becomes a Task base class, and if/when someone wants multiple inheritance, they can revisit this.

    4. So my general principle here is that composition should be preferred over inheritance in almost all circumstances. As a result, when I use a mixin, I never override any methods from the base class it's being mixed into, especially __init__. As you say, there is a way to do this correctly, but I'm always very wary of doing so since it comes with the expectation that everyone who will work on and read this code in the future will know about python's (new style class) MRO semantics. I think this is a hard guarantee to make.

      At the very least, this sounds like a project-level style decision that should be made by the interested parties. Maybe we should hoist this into a pants-devel discussion about general mixin policy?

  3. Nit: s/soruce/source/

  4. Why bother calling get_target? The address will be the same.

  5. I find it's always much nicer if this is sorted.

  6. 
      
DA
DA
JS
  1. 
      
  2. The Scm impl should really be defaulting this value or else you/workspace should do so in an agnostic way, ie None -> scm.commit_id

    1. Hm, I don't really want to require that the Scm be configured and usable at option registration time, so None indeed seems safer.

      commit_id actually has to do work (shell out to rev-parse) -- maybe I should add current_rev_identifier or something to Scm that just returns a constant eg "HEAD" for git.py?

    2. added in latest changes.

  3. 2 blank lines between top level classes

  4. 
      
DT
  1. Ship It!

  2. 
      
DA
PA
  1. Ship It!

  2. 
      
DA
Review request changed

Status: Closed (submitted)

Change Summary:

991b6233b442369be2310ca58fbd7c4b93f72c89
Loading...