Fix BundleAdaptor to BundleProps Conversion

Review Request #4129 — Created July 29, 2016 and submitted

yujiec
pants
3739
pants-reviews
kwlzn, stuhood

This review https://rbcommons.com/s/twitter/r/4057/, implements conversion from BundleAdaptor to BundleProps. However I found an issue in my recent testing in Twitter's internal repo. "bundle" declaration in BUILD takes 4 arguments, "rel_path", "relative_to", "mapper" and "fileset". They were all captrued and stored in BundleAdaptor objects. But the current conversion logic will pass only fileset to newly created BundleProps object and ignore others. This review fixes this issue.

ci green:
https://travis-ci.org/pantsbuild/pants/builds/150737269

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
  1. 
      
  2. src/python/pants/backend/jvm/targets/jvm_app.py (Diff revision 1)
     
     
     
     
     

    using hasattr this way may not be ideal, according to https://hynek.me/articles/hasattr/

    you can try this:

    rel_path = getattr(bundle, 'rel_path', None)

  3. 
      
  1. Agree with Yi: should use getattr with a default.

  2. src/python/pants/engine/legacy/graph.py (Diff revision 1)
     
     

    Please use the decorator that Kris added in https://rbcommons.com/s/twitter/r/4128/ to mark up some existing tests that would cover this behaviour.

    1. I don't think existing integartion tests will cover this behavior. I will write new tests about this particular conversion where new decorator should come handy.

  3. 
      
  1. lgtm!

    btw, mind adding your PR# in the "bugs" field for better tracking?

  2. 
      
  1. thanks Yujie! this is submitted @ f28482ca5dd87f981de73ba8cb47def171e754f3

    please mark this RB closed when you get a chance.

  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks, Yi, Stu and Kris!

Loading...