Fixes to zinc analysis manipulation, and comprehensive tests for it.

Review Request #213 — Created April 11, 2014 and submitted

benjyw
pants
pants-reviews
jsirois
- A unit test for parse/write/split/merge.
- Equality and diffing logic for analyses.
- Fix split and merge bugs found by the test.
- Includes a ~350k tarball of anonymized text analysis files, used as test data.
PANTS_DEV=1 ./pants tests/python/pants_test/tasks/jvm_compile/scala
  • 1
  • 0
  • 0
  • 0
  • 1
Description From Last Updated
Please fix this to be portable. JS jsirois
JS
  1. 
      
  2. twitter.common is now 3rdparty, so I think:
    [stdlib alpha]
    
    [3rdparty alpha]
    
    [local alpha]
    
    That jives with an earlier review and feedback from Patrick.
  3. Documenting the datastructure would be useful - its non-trival and the name doesn't imply it.
  4. This feels a little dirty on 2 fronts: 
    1.) you copy args with the list constructor above and yet side-effect the dicts here.  How about just do _all_ of this in a private copy.
    2.) as the comment says - you do this for tests - it would be nice to add some test infra instead if possible.
  5. just items() for 2+3 compat
  6. unicode(...) defeats your attempts at py2/3 compat - this is enough a pita to get right that a class decorator that expected a special named method to be defined and encapsulated the switching and encoding might make sense.
  7. Funny indents here and below
  8. Please fix this to be portable.
  9. Lots of prints.  I assume you really mean to keep the timing prints since we have no answer for tracking timing regressions, but the rest of these hopefully can go and were just left-overs from you debugging your tests.
  10. This is fine, contextlib.closing works nicely too though.
  11. no \ - you're inside []
  12. This is 3 incompatible.  I'm not sure if we've settled on trying to maintain 2.6-2.7 & 3.3+ compat, but in general effort has been expended in that direction.
  13. 
      
BE
  1. Thanks for the thorough review! I've addressed all your comments below and pushed the fixes. PTAL.
  2. Gah, that was me letting IntelliJ inject the import. Fixed.
  3. Added documentation.
  4. 1. You're correct. That was an error. Fixed.
    
    2. Well it's not just for unit testing, it's also helpful when debugging problems with live analysis files. I'll make sure the comment reflects that. 
  5. Done, but might revisit if this becomes a performance issue.
  6. Ugh, good catch. 
    
    I don't trust myself to get class decorator stuff right, but I did realize (and just verified) that I don't need that unicode() at all. In
    
    ''.join(parts)
    
    The '' will be a unicode due to unicode_literals, and so the entire result will be a unicode even if some (or all) of the parts are strs. 
    
  7. Ooops, good catch. Changed to match the anonymized version in the test analysis files. 
    
    (This string must reflect what's actually in the analysis files, although in this specific case it doesn't matter since we're not examining the dep info for any purpose.)
  8. Removed the ones that print once per each of the 68 test files and left only the ones that print once globally. 
    
    That said, does it matter? You only see these if you run with -s.
    1. Only as a general bad habit - ie: tests that print instead of asserting are a thing that happens.
  9. Didn't know about that. Nice. Fixed.
  10. As before, turns out that unicode() isn't even necessary. Fixed.
  11. 
      
JS
  1. You'll need to `rbt post -r 213` to upload diff 2.  You may or may not need to use --parent depending on how you run your branch life, but the simpler command should do it.
  2. 
      
BE
JS
  1. Ship It!
  2. 
      
BE
Review request changed

Status: Closed (submitted)

Loading...