The backend changes for the hairball split

Review Request #417 — Created May 28, 2014 and submitted

patricklaw
pants
https://github.com/pantsbuild/pants/commits/pl/hairball_cleanup_logical
https://github.com/pantsbuild/pants/issues/5
pants-reviews
benjyw, jsirois, wickman
This is the FULL CR for the entire branch, because apparently submitting a single commit on a branch with RBT is deprecated.  I highly recommend checking out the branch and examining the diffs there.  Also, if you have comments about the mechanical refactors, they would probably work better as regular comments rather than inline comments on those files.
CI passes
  • 1
  • 0
  • 1
  • 0
  • 2
Description From Last Updated
It would be great if register_alias_groups took a type instead of a dict. There is really no need to lose ... JS jsirois
IT
  1. Changes look good to me!
  2. 
      
JS
  1. This looks good to me.  Thanks for being patient.
    
    My only concerns are fairly mild and marked as issues below.
  2. src/python/pants/backend/codegen/BUILD (Diff revision 1)
     
     
    Kill this.
  3. It would be friendly to end of line comment here # globals and then # locals below.
  4. How about register_{goals,commands} - makes it more clear these are side-effecting calls
  5. Just adding 'python' to the list is all you need to do if you were referring to the @manual.builddict below.
  6. src/python/pants/backend/__init__.py (Diff revision 1)
     
     
    What forced this - dev mode somehow?  The pex pipeline is supposed to handle injecting these and any need to add one manually today highlights a bug.
    1. Looks like it wasn't necessary, I think it was me attempting to fix something way earlier in the refactor when I was playing with pkg_resources and source distributions.
  7. This is nice fallout.
  8. It would be great if register_alias_groups took a type instead of a dict.  There is really no need to lose structure here - the groups are well known and finite.  IDE users will also thank you.
    1. I'm not sure what you mean here.  Do you mean an instance of some object that wraps all of these maps?
    2. Yes - exactly:
      return AliasGroups(
        target_aliases={
      
        }
      )
      
  9. 
      
PA
PA
Review request changed

Status: Closed (submitted)

Loading...