Convert validation.assert_list isinstance checking to be lazy

Review Request #2228 — Created May 15, 2015 and submitted

simeon
pants
1550
aac52eb...
pants-reviews
dturner-tw, jsirois, kwlzn, nhoward_tw

Single line alteration - pants.base.validation.assert_list is called frequently and I added laziness to avoid unneccesary function calls. Saw this when profiling and wanted to fix it despite it not being a huge win...

I also added unit tests for this function which doesn't appear to be directly unit before this.

Pull request https://github.com/pantsbuild/pants/pull/1550 runs the Travis CI suite and it all passes.

I can't seem to run the ci script myself but I did run all the unit tests which pass and verified that the new test_validation target was included in the tests that run. One of the tests demonstrates the lazyness of the new version.

MA
  1. It is pretty great to submit a new test class with a one-line patch. This is a drive-by comment to give a ReviewBoard heads-up.

    I saw this in the queue but I don't think there are any current Pants committers in the People tab. You should probably add a few other people to the People group (git log suggests David Turner, jsirois and nhoward).

    Additionally, using your Twitter email address gives RB a hard time with sending emails to pants-reviews. The group won't generally see a review from a @twitter.com email address unless a committer comments on the review itself. If you have future patches in mind, I think that the usual solution is to register with a @twopensource address.

    1. It doesn't matter which email address you use, the address has to be subscribed to the pants-reviews Google group because RBCommons tries to impersonate your email address when sending email to the list. Thanks for mentioning it, I just added Simeon's email to the group.

    2. Unless things have changed - it does matter for @twitter.com addresses. Even if the address matches the pants-reviews signup, a DMARC-like think enabled for the twitter domain causes sends-on-behalf-of to fail.
      The square/foursquare/twitter domains all have some sort of DMARC setup, but I don't know the meaning of these records - suffice to say, Twitter's will caused RBCommons mail sent on behalf of it to be marked invalid / spam - something blackholey - via gmail:

      $ dig -t txt +short twitter.com
      "v=spf1 ip4:199.16.156.0/22 ip4:199.59.148.0/22 ip4:8.25.194.0/23 ip4:8.25.196.0/23 ip4:204.92.114.203 ip4:204.92.114.204/31 ip4:23.21.83.90 include:_spf.google.com include:_thirdparty.twitter.com -all"
      $ dig -t txt +short foursquare.com
      "google-site-verification=Fme-47hce8z3xLMzarNt0E9wVfCtVxxbktlJCaF-5T0"
      "v=spf1 include:_spf.google.com include:aspmx.googlemail.com include:sendgrid.net include:support.zendesk.com ip4:199.38.176.0/22 ~all"
      $ dig -t txt +short square.com
      "v=spf1 include:spf.dynect.net ip4:74.122.184.0/24 ip4:74.122.185.0/24 ip4:74.122.186.0/24 ip4:74.122.187.0/24 ip4:74.122.189.0/24 ip4:74.122.190.0/24 -all"
      
    3. Although its not too hard to figure out, for example, following the twitter.com include:

      $ dig -t txt +short _thirdparty.twitter.com
      "v=spf1 ip4:96.43.144.64/31 ip4:96.43.148.64/31 ip4:182.50.78.64/28 ip4:204.14.232.64/28 ip4:204.14.234.64/28 include:mail.zendesk.com -all"
      

      So I think these domains would need to add an include for rbcommons.com:

      $ dig -t txt +short rbcommons.com
      "keybase-site-verification=G9SGiteLZT8sW11nv_DLa2HzyWV6J4LmHGRaAW7mjCo"
      "v=spf1 include:mailgun.org ~all"
      
    4. I opened a ticket requesting an @twopensource email address. I'll subscribe that to the pants-review group and set it in my rbcommons profile as soon as I get it. Thanks for the heads up Mateo.

  2. s/2014/2015/

    1. Thanks - updated.

  3. 
      
ZU
  1. 
      
  2. I've not seen this before, nor the monkeypatching pattern in python before. Why are you aliasing assert_list this way instead of importing it as from pants.base.validation import assert_list ?

    1. Fair point - I originally was repeatedly calling test_validate.assert_list in my tests - I thought of this line as aliasing the name to give me less typing. It could also be an additional import statement. However - given John Sirois' observation that isinstance takes a tuple of types I'm going to kill the monkeypatching "lazyness test" anyways in my update. Thanks for looking at this.

  3. 
      
JS
  1. 
      
  2. src/python/pants/base/validation.py (Diff revision 1)
     
     
    How about leverage the lib and `isinstance(val, allowable)`.
    1. Obviously correct! I could also remove the test verifying the number of calls to isinstance - no need to demonstrate lazyness in this case.

      I'll note that in this case allowable MUST be a tuple - a list of types would cause isinstance to raise a TypeError. The default value is a tuple and grepping around the code base I see only one instance of explicitly setting this value.

      I'll modify the patch and make sure the tests still pass - thanks John.

  3. 
      
SI
SI
DT
  1. Ship It!
  2. 
      
JS
  1. Ship It!
  2. 
      
JS
  1. In master @ https://github.com/pantsbuild/pants/commit/a366cf2cb3afce2f8560a596e3dc199ee2ed5320
    Please mark this review as submitted.
  2. 
      
SI
Review request changed

Status: Closed (submitted)

Change Summary:

In master @ https://github.com/pantsbuild/pants/commit/a366cf2cb3afce2f8560a596e3dc199ee2ed5320

Loading...