Clean-up new API interface

Review Request #2845 — Created Feb. 8, 2012 and discarded

Information

RBTools

Reviewers

The legacy code is unstructured and it seems that noone has ever run it. Starting from this patch I will clean server interface until it becomes suitable for making HTTP requests. I intend to keep the chunks as small as possible in order to simplify the review process and speed-up acceptances, so some-times the logic may seem uncompleted.

 
Description From Last Updated

These comments say "list", but the default arguments are Python dicts. Also, maybe I'm not understanding this correctly - but …

mike_conleymike_conley

For NotImplementedError, it's not necessary to include a message unless it has some specific instructions. The current message just repeats …

chipx86chipx86

For the purposes of clarity, it'd be nice to keep what we had before where the results of raw_input and …

chipx86chipx86
MB
MB
mike_conley
  1. Hey Alexander:
    
    While I understand the impulse to keep the commits small, it's difficult to review the logic of the code without the full picture of what you're trying to do.
    
    For the most part, the code itself looks OK - but without a clearer picture of your overall strategy, it's hard to say whether or not what you're doing makes sense.
    
    I do have one comment though - see below.
    
    -Mike
    1. The idea is to implement new API using a code generator. Since the previous approach differed I had to cleanup/remove the most of the legacy code. This could help to understand https://groups.google.com/d/topic/reviewboard-dev/HFm7rz4QE2k/discussion 
      
      I used to make another patch where clean-ups were mixed with new code and it was a real mess.
  2. rbtools/api/serverinterface.py (Diff revision 3)
     
     
     
     
     
    These comments say "list", but the default arguments are Python dicts.
    
    Also, maybe I'm not understanding this correctly - but are you expecting fields / files to be lists of tuples?
    
    Like:
    
    [('someKey', 'someFileName', 'Some content!')]
    
    for files?
  3. 
      
MB
chipx86
  1. Seems reasonable, but I want to be sure that everything continues to work with this change?
  2. rbtools/api/utils.py (Diff revision 4)
     
     
    For NotImplementedError, it's not necessary to include a message unless it has some specific instructions. The current message just repeats the purpose of the exception, so I wouldn't include anything at that point.
  3. rbtools/api/utils.py (Diff revision 4)
     
     
    For the purposes of clarity, it'd be nice to keep what we had before where the results of raw_input and getpass are put into variables, and then returned. The reason being that each of these is a pretty hefty operation (block for input) and should be read as two operations rather than one operation where things are just returned.
  4. 
      
MB
MB
Review request changed

Status: Discarded

Loading...