- Diff:
-
Revision 2 (+677 -1)
HostingService API
Review Request #5602 — Created March 9, 2014 and discarded
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. |
chipx86 | |
Where is this? |
chipx86 | |
Blank line between these. |
chipx86 | |
type is a reserved word. You'll want to use repository_type probably. |
chipx86 | |
Blank line between these. For the setting of the variables, we'd prefer one per line. |
chipx86 | |
Blank line between these. |
chipx86 | |
No space before """ |
chipx86 | |
We do this here, but not below or above. Is that intentional? |
chipx86 | |
No blank line here. |
chipx86 | |
No trailing period in test docstrings. Same on all test docstrings below. |
chipx86 | |
You shouldn't use u for strings, since it's implied. Make sure that's fixed everywhere. |
chipx86 | |
Make sure you add the following as the first line, with a blank line below: from __future__ import unicode_literals Same … |
chipx86 | |
Must be in alphabetical order. |
chipx86 | |
Alignment issue. |
chipx86 | |
This class needs a docstring. Note that the docstring is what will go into the actual generated API docs on … |
chipx86 | |
We always use this form: item_child_resources = [ blah, ] |
chipx86 | |
This will also turn into docs on the website. Read through what we say on other resources, and try to … |
chipx86 | |
Should be: list_items = [ {...} for name, class_object in six.iteritems(services) ] |
chipx86 | |
I suggest defining a payload variable in these, and then returning the 200, payload after the conditional. |
chipx86 | |
Alphabetical order. |
chipx86 | |
The first import here is in the wrong group. |
chipx86 | |
I think what we want to do is make this a child of HostingServiceAccountResource, and not HostingServiceResource. By doing this, … |
chipx86 | |
All the same docstring comments and GET <item> apply to this file. |
chipx86 | |
I don't really see a reason to have this function. This can be done inline instead. |
chipx86 | |
If we move this class in the hierarchy, we can also remove this required field, since it'll be part of … |
chipx86 | |
This should be optional here, and should only exist if the service requires it. You can check when we process … |
chipx86 | |
This doesn't mean anything from an API standpoint. |
chipx86 | |
Comments must be in sentence case with a trailing period. Same with all others. |
chipx86 | |
We don't want to supply a dictionary resource. All objects we put in a payload should be the objects themselvs, … |
chipx86 | |
No blank line here. |
chipx86 | |
Each part of a comment should be in sentence casing, with a period at the end. You probably won't need … |
chipx86 | |
Or here. |
chipx86 | |
Should a new error be created in the API errors for when the HostingService throws a PlanError? |
OL olessia | |
Or here. |
chipx86 | |
Alignment is weird. This should be: hosting_service_repository_list_mimetype = \ _build_mimetype('...') |
chipx86 | |
This is third-party, so it should go in the same group as django and djblets. |
chipx86 | |
Shouldn't add this blank line. |
chipx86 | |
A mixin only makes sense if the same set of tests will be used in many places. I don't think … |
chipx86 | |
Should be a single line. |
chipx86 | |
Space after , |
chipx86 | |
I don't see these being used. |
chipx86 | |
This needs to go by djblets. |
chipx86 | |
This line is too long. Should be more like: return resources....get_list_url( hosting_service=..., local_site_name=...) |
chipx86 | |
How about something like this: if plan not in [pair[0] for pair in self.plans]: That way we aren't adding yet … |
david | |
The first line of this file should be: from __future__ import unicode_literals |
david | |
Don't remove this line. |
david | |
Why did you make this change? |
david | |
Please add a blank line between these two. |
david | |
This should use from django.utils import six instead (we want to use the one that they ship, rather than depending … |
david | |
Please wrap to 80 columns. |
david | |
Please wrap to 80 columns. |
david | |
Can you add a blank line between these two? |
david | |
Please revert this change. |
david | |
Can you re-wrap this to make it as compact as possible? It might be better to put all args on … |
david | |
Alignment is off here. |
david | |
This line should be indented another 4 spaces because there are arguments to get_hosting_service_repository_item_url |
david | |
Alignment is off here. |
david |
- Testing Done:
-
~ tests added to webapi tests
~ tests added to webapi:
+ tests/test_hosting_service + tests/test_hosting_service_repository, - Diff:
-
Revision 3 (+652 -1)
-
-
-
-
-
-
-
-
-
-
-
-
-
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.
-
-
-
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?
-
-
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.
-
-
I suggest defining a
payload
variable in these, and then returning the200, payload
after the conditional. -
-
-
-
-
-
-
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).
-
Alignment is weird. This should be:
hosting_service_repository_list_mimetype = \ _build_mimetype('...')
-
-
-
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.
-
-
-
-
This line is too long. Should be more like:
return resources....get_list_url( hosting_service=..., local_site_name=...)
- Description:
-
~ API for getting HostingServices and HostingSeerviceRetositories
~ API for getting HostingServices and HostingServiceRetositories
- Diff:
-
Revision 4 (+828 -2)
-
-
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.
-
If we move this class in the hierarchy, we can also remove this required field, since it'll be part of the URL.
-
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). -
-
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. -
-
-
-
-
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.
-
-
-
-
-
This should use
from django.utils import six
instead (we want to use the one that they ship, rather than depending on another package). -
-
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.
-
-
-
-
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.
-
-
This line should be indented another 4 spaces because there are arguments to
get_hosting_service_repository_item_url
-
-
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.
- Diff:
-
Revision 6 (+1078 -1)