BuildFile refactoring: remove usages and deprecate of BuildFile's family, ancestors, siblings and descendants methods

Review Request #3368 — Created Jan. 23, 2016 and submitted

tabishev
pants
tabishev/build_file_methods_refactoring
2833
pants-reviews
dturner-tw, patricklaw, stuhood, zundel

In order to implement pants_build_ignore option I need to have as less as possible places where I need to pass this option to.

This RB remove usages/deprecate all returning BuildFiles methods except scan_project_tree_build_files, get_project_tree_build_files_family in BuildFile which sounds like sufficient interface to get BuildFiles in general.

https://travis-ci.org/ttim/pants/builds/104780568

  • 0
  • 0
  • 3
  • 1
  • 4
Description From Last Updated
ZU
  1. 
      
  2. It seems to me that passing around the spec_excludes option is a burden. Did you consider moving spec_excludes to a subsystem?

    1. Current plan is to move future pants_build_ignore option to BuildFileAddressMapper, and noone (or almost noone) except BuildFileAddressMapper will know about it.

  3. usually we omit the 'len()' test for lists:

    if not build_files:
    

    Is there a reason its needed here?

  4. 
      
PA
  1. 
      
  2. src/python/pants/base/build_file.py (Diff revision 1)
     
     

    Have you double checked with all of the relevant parties whether this is an okay deprecation cycle? It's pretty aggressive.

    I think 4sq is fine with this, but you should check with Square and whoever you need to at Twitter in case you haven't.

    1. It was 4 versions ahead before release, which now only 3 versions ahead. Should I move it forward a little bit?

      Relevant parties looks ok with this change.

  3. Unfortunate to see "private" access here. Should address_mapper expose an interface for this purpose? It doesn't have to explicitly talk about BUILD files, it could just abstract it away to talk about non-empty spec paths (which in this case happens to always correspond to a BUILD file, but in the future it might be more abstract).

    1. =patrick. We already have a large surface area, and it's only going to be manageable in the future if we insist on a strict "no external references to underscore-prefixed symbols" rule.

  4. 
      
TA
TA
PA
  1. 
      
  2. There should be some indication on the base AddressMapper class that this method is expected to be implemented by subclasses, and the contract of the method says to raise AddressLookupError (or a subclass) when the spec isn't found.

    1. What is a base class for BuildFileAddressMapper? Looks like AddressMapper exists only in new engine?

    2. Huh, I thought there was another one! Nevermind then.

  3. Comment is superfluous

  4. 
      
TA
TA
TA
ST
  1. 
      
  2. "does not contain any BUILD files"... but we can clean it up in a future review.

  3. 
      
TA
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as 3442c3da20317e9e3948639bc96481124d74bf43

Loading...