Adds DefaultOrderedDict to twitter.common.collections

Review Request #697 — Created July 16, 2014 and discarded

zundel
commons
zundel/add-defaultordereddict
306
pants-reviews
ity, wickman
This new class is meant to be a substitute for defaultdict(). I wanted to use it in pants because I'm trying to get to the bottom of what seems to be non-deterministic behavior in ordering the classpath for jvm compiles between different machines both working from head of master in the same repo.

I also added some tests for OrderedDict while I was at it.

One part of the design I wasn't so sure about was how to pass in the function for __missing__().  I used a setter and a create() factory method to take the place of passing a function to the constructor because I believe OrderedDict() already has a use for the constructor arg.
Added unit tests.  CI is started at https://travis-ci.org/twitter/commons/builds/30104031
  • 0
  • 0
  • 8
  • 0
  • 8
Description From Last Updated
PA
  1. 
      
  2. Any reason not to use the `callable` predicate here?
    1. I read in StackOverflow: http://stackoverflow.com/questions/624926/how-to-detect-whether-a-python-variable-is-a-function  that callable() was deprecated.
      
      I just checked the python 3 docs which says it was removed then put back!  https://docs.python.org/3/library/functions.html?highlight=callable#callable
    2. I hadn't realized that!  But we don't support 3.0-3.2 anyway, so I guess `callable` is still preferred?
  3. 
      
IT
  1. thank you for the extensive tests - I think it would be good to get this in anyway and we can copy this dependency alongwith the others as we talked about in the summit. 
  2. indent at 4, instead of 2? throughout.
  3. assert not key in d (has_key I think is deprecated 3.x onwards). same throughout.
  4. 'c', 'o', 
    
    .. and throughout
  5. 
      
ZU
  1. I ran auto-format in IntelliJ throughout.
  2. orderedict.py was using an indent of 4 and I thought I was following an existing convention, but after your comment I went back and discovered that that file was an exception and 2 character indent was used everywhere else.  
  3. 
      
ZU
ZU
  1. ping. My plan is to extract the OrderedDefaultDict and put it in pants, so if you don't want to mess with this review, let me know.
  2. 
      
IT
  1. Ship It!
    1. Thanks Ity.  Remember that I don't have committer privs on this repo so someone will have to do it in my behalf.
  2. 
      
ZU
Review request changed

Status: Discarded

Loading...