Convert JarRule and JarRules to use Payload to help fingerprint its configuration

Review Request #2096 — Created April 19, 2015 and submitted

zundel
pants
zundel/jar-rules-to-payload
1434
a7caa00...
pants-reviews
dturner-tw, patricklaw
Convert JarRule and JarRules to use Payload to help fingerprint its configuration

Unit tests already cover the fingerprinting of these types, CI run at https://travis-ci.org/pantsbuild/pants/builds/59110202

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
ZU
  1. This is in response to David's comments in https://rbcommons.com/s/twitter/r/2084/, I think that re-using Payload is a more foolproof way to make sure we don't fall into the pitfall I did when trying to fingerprint a dict by hand.

  2. 
      
PA
  1. 
      
  2. What's the motivation behind freezing here? Normally the only place we freeze is in the base Target.__init__ but of course this isn't a Target subclass. I wonder if FingerprintedMixin should do the freezing?

    Thinking about this a bit, I think what you're doing here is probably best for now since you know this is the base class, but maybe in the future we should consider other options for composing target payloads with payloads owned by FingerprintedMixin members of that target.

    1. I guess we don't have to freeze. The target freeze should capture this as well. As for moving stuff into FingerprintedMixin, I thought about it, but you've told me before you didn't want to add __init__() to mixins.

    2. Yeah, I've come around a bit on using non-pure mixins, because we've seen some benefit from carefully using mixins that override methods and call super in, e.g., options registration. If we're very careful about keeping mixins on the left and always calling super properly, I think it ends up paying for the complexity. That being said, __init__ is a little bit more scary to use like that, so I've been playing wait-and-see until there's a case that seems to justify it.

  3. 
      
ZU
ZU
ZU
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks Patrick. Commit b225ea4

DT
  1. 
      
  2. Looks like a bogus duplicated line.

    1. Thanks David. I've added this change to another PR I already had open.

  3. 
      
Loading...