Add repositories/<id>/branches/ and repositories/<id>/commits resources.
Review Request #4265 — Created June 26, 2013 and submitted
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 … |
 chipx86
|
|
|
I think we need docstrings that detail what these do and what's the format of the results. |
chipx86
|
|
|
Should add docs here too. |
chipx86
|
|
|
Missing period. |
chipx86
|
|
|
Can you add constants for this and the other cache key? |
chipx86
|
|
|
We have this function, but then we build cache keys inline in the other functions. Any reason we don't just … |
chipx86
|
|
|
The """ should be on the next line. |
chipx86
|
|
|
Missing period. |
chipx86
|
|
|
The """ should be on the next line. Also, the webapi docs should be more explicit about what the caller … |
chipx86
|
|
|
""" on the next line. |
chipx86
|
|
|
Trailing period. |
chipx86
|
|
|
This would read better by doing 'for commit in commits', and then accessing commit instead of commits[i]. |
chipx86
|
|
|
Should inherit from object. |
chipx86
|
|
|
Should inherit from object. |
chipx86
|
|
|
Just noticed, the "trunk, _" and similar below overwrite the ugettext alias "_" in this scope. We should probably avoid … |
chipx86
|
-
-
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.
-
reviewboard/hostingsvcs/github.py (Diff revision 3) 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.
-
reviewboard/hostingsvcs/service.py (Diff revision 3) I think we need docstrings that detail what these do and what's the format of the results.
-
-
-
reviewboard/scmtools/models.py (Diff revision 3) Can you add constants for this and the other cache key?
-
reviewboard/scmtools/models.py (Diff revision 3) 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?
-
-
-
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?
-
reviewboard/webapi/resources.py (Diff revision 3) 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.
-
-
-
reviewboard/webapi/resources.py (Diff revision 3) This would read better by doing 'for commit in commits', and then accessing commit instead of commits[i].
-
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.
-
-
-
reviewboard/scmtools/svn.py (Diff revision 4) 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.