HostingService API
Review Request #5602 — Created March 9, 2014 and discarded
Information | |
---|---|
olessia | |
Review Board | |
master | |
Reviewers | |
students | |
API for getting HostingServices and HostingServiceRetositories
tests added to webapi:
tests/test_hosting_service
tests/test_hosting_service_repository,
Description | From | Last Updated |
---|---|---|
No blank line here. |
|
|
Where is this? |
|
|
Blank line between these. |
|
|
type is a reserved word. You'll want to use repository_type probably. |
|
|
Blank line between these. For the setting of the variables, we'd prefer one per line. |
|
|
Blank line between these. |
|
|
No space before """ |
|
|
We do this here, but not below or above. Is that intentional? |
|
|
No blank line here. |
|
|
No trailing period in test docstrings. Same on all test docstrings below. |
|
|
You shouldn't use u for strings, since it's implied. Make sure that's fixed everywhere. |
|
|
Make sure you add the following as the first line, with a blank line below: from __future__ import unicode_literals Same … |
|
|
Must be in alphabetical order. |
|
|
Alignment issue. |
|
|
This class needs a docstring. Note that the docstring is what will go into the actual generated API docs on … |
|
|
We always use this form: item_child_resources = [ blah, ] |
|
|
This will also turn into docs on the website. Read through what we say on other resources, and try to … |
|
|
Should be: list_items = [ {...} for name, class_object in six.iteritems(services) ] |
|
|
I suggest defining a payload variable in these, and then returning the 200, payload after the conditional. |
|
|
Alphabetical order. |
|
|
The first import here is in the wrong group. |
|
|
I think what we want to do is make this a child of HostingServiceAccountResource, and not HostingServiceResource. By doing this, … |
|
|
All the same docstring comments and GET <item> apply to this file. |
|
|
I don't really see a reason to have this function. This can be done inline instead. |
|
|
If we move this class in the hierarchy, we can also remove this required field, since it'll be part of … |
|
|
This should be optional here, and should only exist if the service requires it. You can check when we process … |
|
|
This doesn't mean anything from an API standpoint. |
|
|
Comments must be in sentence case with a trailing period. Same with all others. |
|
|
We don't want to supply a dictionary resource. All objects we put in a payload should be the objects themselvs, … |
|
|
No blank line here. |
|
|
Each part of a comment should be in sentence casing, with a period at the end. You probably won't need … |
|
|
Or here. |
|
|
Should a new error be created in the API errors for when the HostingService throws a PlanError? |
OL olessia | |
Or here. |
|
|
Alignment is weird. This should be: hosting_service_repository_list_mimetype = \ _build_mimetype('...') |
|
|
This is third-party, so it should go in the same group as django and djblets. |
|
|
Shouldn't add this blank line. |
|
|
A mixin only makes sense if the same set of tests will be used in many places. I don't think … |
|
|
Should be a single line. |
|
|
Space after , |
|
|
I don't see these being used. |
|
|
This needs to go by djblets. |
|
|
This line is too long. Should be more like: return resources....get_list_url( hosting_service=..., local_site_name=...) |
|
|
How about something like this: if plan not in [pair[0] for pair in self.plans]: That way we aren't adding yet … |
|
|
The first line of this file should be: from __future__ import unicode_literals |
|
|
Don't remove this line. |
|
|
Why did you make this change? |
|
|
Please add a blank line between these two. |
|
|
This should use from django.utils import six instead (we want to use the one that they ship, rather than depending … |
|
|
Please wrap to 80 columns. |
|
|
Please wrap to 80 columns. |
|
|
Can you add a blank line between these two? |
|
|
Please revert this change. |
|
|
Can you re-wrap this to make it as compact as possible? It might be better to put all args on … |
|
|
Alignment is off here. |
|
|
This line should be indented another 4 spaces because there are arguments to get_hosting_service_repository_item_url |
|
|
Alignment is off here. |
|
Testing Done: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+652 -1) |
-
-
-
-
-
reviewboard/hostingsvcs/beanstalk.py (Diff revision 3) type
is a reserved word. You'll want to userepository_type
probably. -
reviewboard/hostingsvcs/beanstalk.py (Diff revision 3) Blank line between these.
For the setting of the variables, we'd prefer one per line.
-
-
-
reviewboard/hostingsvcs/github.py (Diff revision 3) We do this here, but not below or above. Is that intentional?
-
-
reviewboard/hostingsvcs/tests.py (Diff revision 3) No trailing period in test docstrings.
Same on all test docstrings below.
-
reviewboard/hostingsvcs/tests.py (Diff revision 3) You shouldn't use
u
for strings, since it's implied. Make sure that's fixed everywhere. -
reviewboard/webapi/resources/hosting_service.py (Diff revision 3) Make sure you add the following as the first line, with a blank line below:
from __future__ import unicode_literals
Same for every newly introduced file.
-
-
-
reviewboard/webapi/resources/hosting_service.py (Diff revision 3) This class needs a docstring.
Note that the docstring is what will go into the actual generated API docs on reviewboard.org, so it should be intended for reading by developers trying to learn about the API.
Also, I don't see any handler for GETing an item on the list?
-
reviewboard/webapi/resources/hosting_service.py (Diff revision 3) We always use this form:
item_child_resources = [ blah, ]
-
reviewboard/webapi/resources/hosting_service.py (Diff revision 3) This will also turn into docs on the website. Read through what we say on other resources, and try to come up with some useful text that someone stumbling across the resource will find useful.
-
reviewboard/webapi/resources/hosting_service.py (Diff revision 3) Should be:
list_items = [ {...} for name, class_object in six.iteritems(services) ]
-
reviewboard/webapi/resources/hosting_service.py (Diff revision 3) I suggest defining a
payload
variable in these, and then returning the200, payload
after the conditional. -
-
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 3) The first import here is in the wrong group.
-
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 3) All the same docstring comments and GET <item> apply to this file.
-
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 3) I don't really see a reason to have this function. This can be done inline instead.
-
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 3) This doesn't mean anything from an API standpoint.
-
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 3) Comments must be in sentence case with a trailing period.
Same with all others.
-
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 3) We don't want to supply a dictionary resource. All objects we put in a payload should be the objects themselvs, which will be properly serialized automatically (provided the object is associated with a resource).
-
reviewboard/webapi/tests/mimetypes.py (Diff revision 3) Alignment is weird. This should be:
hosting_service_repository_list_mimetype = \ _build_mimetype('...')
-
reviewboard/webapi/tests/mixins.py (Diff revision 3) This is third-party, so it should go in the same group as django and djblets.
-
-
reviewboard/webapi/tests/mixins.py (Diff revision 3) A mixin only makes sense if the same set of tests will be used in many places. I don't think that's the case here. These tests should all instead go into the specific unit test.
-
-
-
reviewboard/webapi/tests/test_hosting_service_repository.py (Diff revision 3) This needs to go by djblets.
-
reviewboard/webapi/tests/urls.py (Diff revision 3) This line is too long. Should be more like:
return resources....get_list_url( hosting_service=..., local_site_name=...)
-
-
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 3) Should a new error be created in the API errors for when the HostingService throws a PlanError?
Description: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+828 -2) |
-
-
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 3) I think what we want to do is make this a child of HostingServiceAccountResource, and not HostingServiceResource.
By doing this, we already have automatic filtering of account, since that data will be stored on the account.
-
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 3) If we move this class in the hierarchy, we can also remove this required field, since it'll be part of the URL.
-
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 3) This should be optional here, and should only exist if the service requires it. You can check when we process the service below, and return an appropriate error if missing (probably using the
MISSING_ATTRIBUTE
error). -
-
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 3) Each part of a comment should be in sentence casing, with a period at the end.
You probably won't need this, though. What you'd want to do, once this is moved in the hierarchy, is to make use of
HostingServiceAccountResource.get_object
. That should give you the account and its HostingService, and you could follow our other resources to see how we handle those errors. -
-
-
-
-
reviewboard/hostingsvcs/github.py (Diff revision 5) How about something like this:
if plan not in [pair[0] for pair in self.plans]:
That way we aren't adding yet another place where these are enumerated.
-
reviewboard/hostingsvcs/repository.py (Diff revision 5) The first line of this file should be:
from __future__ import unicode_literals
-
-
-
reviewboard/webapi/resources/hosting_service.py (Diff revision 5) Please add a blank line between these two.
-
reviewboard/webapi/resources/hosting_service.py (Diff revision 5) This should use
from django.utils import six
instead (we want to use the one that they ship, rather than depending on another package). -
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 5) Please wrap to 80 columns.
-
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 5) Just a personal preference, but one thing I like doing in situations like these is making the initial condition as simple as possible. In this case, that would mean flipping these to be:
if plan: return INVALID_ATTRIBUTE, ... else: return MISSING_ATTRIBUTE, ...
If you don't agree, feel free to ignore this. There are more instances below of the same kind of change that you could make.
-
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 5) Please wrap to 80 columns.
-
reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 5) Can you add a blank line between these two?
-
-
reviewboard/webapi/tests/test_hosting_service_repository.py (Diff revision 5) Can you re-wrap this to make it as compact as possible?
It might be better to put all args on the next line and indent by 4 spaces.
-
reviewboard/webapi/tests/test_hosting_service_repository.py (Diff revision 5) Alignment is off here.
-
reviewboard/webapi/tests/test_hosting_service_repository.py (Diff revision 5) This line should be indented another 4 spaces because there are arguments to
get_hosting_service_repository_item_url
-
reviewboard/webapi/tests/test_hosting_service_repository.py (Diff revision 5) Alignment is off here.
-
There are also a few things unrelated to the code: * You have 3 issues from previous reviews which are still marked as open, but look like they're fixed in your code. Can you verify and close them? * There's a typo in your description ("HostingServiceRetositories"). Your description should also be fleshed out a lot more. * You've added a .DS_Store file to reviewboard/webapi. Please get rid of this from your change.