Add repositories/<id>/branches/ and repositories/<id>/commits resources.

Review Request #4265 — Created June 26, 2013 and submitted

Information

Review Board
master

Reviewers

Add repositories/<id>/branches/ and repositories/<id>/commits resources.

This change adds APIs for getting a list of all branches on a repository, and a
(paginated) list of commits backwards in time from a given starting point. These
are currently implemented for the SVN SCMTool and the GitHub hosting service.
- Ran unit tests.
- Used these in conjunction with my post-commit implementation.
Description From Last Updated

Can we factor this out into a common function? I think it'd be worth adding a new _api_get function that …

chipx86chipx86

I think we need docstrings that detail what these do and what's the format of the results.

chipx86chipx86

Should add docs here too.

chipx86chipx86

Missing period.

chipx86chipx86

Can you add constants for this and the other cache key?

chipx86chipx86

We have this function, but then we build cache keys inline in the other functions. Any reason we don't just …

chipx86chipx86

The """ should be on the next line.

chipx86chipx86

Missing period.

chipx86chipx86

The """ should be on the next line. Also, the webapi docs should be more explicit about what the caller …

chipx86chipx86

""" on the next line.

chipx86chipx86

Trailing period.

chipx86chipx86

This would read better by doing 'for commit in commits', and then accessing commit instead of commits[i].

chipx86chipx86

Should inherit from object.

chipx86chipx86

Should inherit from object.

chipx86chipx86

Just noticed, the "trunk, _" and similar below overwrite the ugettext alias "_" in this scope. We should probably avoid …

chipx86chipx86
david
david
chipx86
  1. General logic looks fine. Most of my comments have to do with documentation and ensuring consistency.
    
    This is missing webapi docs additions (the *.txt files, based on class names, and the addition to index.txt).
    
    You can do a 'make html' to see that the docs look right.
  2. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
    I still think that a class is going to be better than a dictionary, so that it's easy to look at one definition and know all the possible values. It's an interface all implementations must follow. I don't think a dictionary communicates any of this well.
  3. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Can we factor this out into a common function?
    
    I think it'd be worth adding a new _api_get function that does the http_get, simplejson.loads, and try/except. Callers could then just call that and deal immediately with the result.
  4. reviewboard/hostingsvcs/service.py (Diff revision 3)
     
     
     
     
     
    Show all issues
    I think we need docstrings that detail what these do and what's the format of the results.
  5. reviewboard/scmtools/core.py (Diff revision 3)
     
     
     
     
     
    Show all issues
    Should add docs here too.
  6. reviewboard/scmtools/models.py (Diff revision 3)
     
     
    Show all issues
    Missing period.
  7. reviewboard/scmtools/models.py (Diff revision 3)
     
     
    Show all issues
    Can you add constants for this and the other cache key?
  8. reviewboard/scmtools/models.py (Diff revision 3)
     
     
     
    Show all issues
    We have this function, but then we build cache keys inline in the other functions. Any reason we don't just go with one way or the other?
    1. I don't know that it's worth changing--the reason for this function is that I use it from other code in a future change (to save an API call when possible), but the inline ones are only used for memoization.
  9. reviewboard/scmtools/models.py (Diff revision 3)
     
     
    Show all issues
    The """ should be on the next line.
  10. reviewboard/scmtools/models.py (Diff revision 3)
     
     
    Show all issues
    Missing period.
  11. reviewboard/scmtools/svn.py (Diff revision 3)
     
     
    Is there any standard in terms of what the limit is per call? Can the get_commits functions take one, or do some services impose a cap?
    1. GitHub always returns 30 results unless you give it "since" and/or "until" parameters, which are dates instead of a numeric count. I'll change this one to match but I think we may end up at the mercy of other services' APIs.
  12. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    The """ should be on the next line.
    
    Also, the webapi docs should be more explicit about what the caller should expect. Right now, there's nothing showing what the caller will get back from this API. I want to make sure any new APIs we add have detailed docs.
    
    Same applies to the commits below.
  13. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    """ on the next line.
  14. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    Trailing period.
  15. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    This would read better by doing 'for commit in commits', and then accessing commit instead of commits[i].
  16. 
      
david
chipx86
  1. This looks great. I have only a couple small comments, but I don't need to re-review it once those are taken care of.
  2. reviewboard/scmtools/core.py (Diff revision 4)
     
     
    Show all issues
    Should inherit from object.
  3. reviewboard/scmtools/core.py (Diff revision 4)
     
     
    Show all issues
    Should inherit from object.
  4. reviewboard/scmtools/svn.py (Diff revision 4)
     
     
    Show all issues
    Just noticed, the "trunk, _" and similar below overwrite the ugettext alias "_" in this scope. We should probably avoid that, in case it bites us later.
  5. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to master (bf6f606).