Add 'transitve' and 'scope' attributes to export of target

Review Request #3845 — Created May 9, 2016 and submitted

Eric Ayers
pants
zundel/add-scope-transitive-export-respin
3386, 3396
3f56eb0...
pants-reviews
gmalmquist, peiyu, stuhood, wisechengyi

These attributes were introduced in https://rbcommons.com/s/twitter/r/3582/

Updated unit tests, added integration test
CI build: https://travis-ci.org/pantsbuild/pants/builds/129422787

  • 0
  • 0
  • 1
  • 1
  • 2
Description From Last Updated
Peiyu Wang
  1. Ship It!
  2. 
      
Stu Hood
  1. 
      
  2. Reaching into the payload is a bit strange. Should this property be exposed on the Target?

    I guess the original idea was that since this was experimental, it wasn't supposed to be exposed... if it's actually seeing some use, then maybe it's worth graduating/stabilizing.

  3. 
      
Eric Ayers
Eric Ayers
Nick Howard (Twitter)
  1. A couple things I noticed while catching up on my email.

  2. I think it'd be nice if this set the value to True in the true case, since to check this, you'd need to do

    if 'transitive' in info and info['transitive'] is not False:
    ...

    Which feels a little confusing to me.

    If it were set to true, you wouldn't have to do an existence check provided the version is what you expected.

    if info['transitive']:
    ...
    else:
    ...

    1. I thought about it, but almost every target has the value set to true and this would just be adding extra cruft to the export file. If you create a target and do nothing, you get transitive=true. You have to explicitly set it to false.

    2. So, maybe marking it as intransitive, rather than transitive would make more sense? The "missing => True", "existing && value == False => False" feels weird to me, but that's just my gut feeling.

    3. I reverted and re-opened the change. I decided to just unconditionally output the value seeing as that might be less surprising.
      I did not change the name of the field. I feel like making a change might make it more difficult to trace back to the code that creates it since we call the property transitive in the code.

  3. Note, if current_target.scope is None, scope_string will be 'None', which I don't think was the intended behavior.

    Could you add a test covering the None case?

    1. Target's self._scope is always initialized in the target base class. Therefore, doesn't a None scope just mean that pants is broken?

    2. Hmm. You're right. Then, the if shouldn't be necessary, because str(...) should always return something truthy. If it doesn't, there's a bug in how scopes work. I don't think it's this code's responsibility to guard against that.

    3. Removed the if() test.

  4. 
      
Eric Ayers
Eric Ayers
Garrett Malmquist
  1. 
      
  2. nit: typo in spelling of "intransitive" in this test name

  3. 
      
Eric Ayers
Nick Howard (Twitter)
  1. Ship It!
  2. 
      
Eric Ayers
Review request changed

Status: Closed (submitted)

Change Summary:

Re-committed to master @ e18807300813296c51b8b7fc4b124f75deb1459e and slated for 1.0.1 release

Loading...