-
-
-
-
I think we should be explicit about what we pull out. As discussed, we'll also need to transform
url
topath
and stuff. -
Docstrings are always in one of the following two forms:
"""One-line summary."""
or:
"""One-line summary Multi-line description. """
-
-
I think any extra data should be explicitly provided in an
extra_data
field in the constructor, and stored in a matching field in the class. -
Same comment above about the docstring.
Documentation should always be clear about what's being returned. Instead of "maybe instances," the body of the docstring should say "This will return a list of zero or more HostingServiceRepository objects."
-
-
-
This looks indented too far.
Also, the trailing brace should match the beginning of the line opening the brace (so, same level as
repos
) -
HostingService: remote repositories for user
Review Request #5432 — Created Feb. 8, 2014 and discarded
Add a basic representation of a remote repository, add get_remote_repositories to HostingService and its implementation for GitHub.
Test added to hostingsvcs tests.
Description | From | Last Updated |
---|---|---|
This should be included above with the other reviewboard imports, listed alphabetically. |
chipx86 | |
When there's only a summary, the trailing """ should be on the first line. |
chipx86 | |
I think we should be explicit about what we pull out. As discussed, we'll also need to transform url to … |
chipx86 | |
Docstrings are always in one of the following two forms: """One-line summary.""" or: """One-line summary Multi-line description. """ |
chipx86 | |
As discussed, hould be path and mirror_path. |
chipx86 | |
I think any extra data should be explicitly provided in an extra_data field in the constructor, and stored in a … |
chipx86 | |
Same comment above about the docstring. Documentation should always be clear about what's being returned. Instead of "maybe instances," the … |
chipx86 | |
Same comment about import order. |
chipx86 | |
This function needs a docstring matching the format of the previous docstrings. |
chipx86 | |
This looks indented too far. Also, the trailing brace should match the beginning of the line opening the brace (so, … |
chipx86 | |
This should specifically compare the fields we care about. |
chipx86 | |
There should still be two blank lines here. |
chipx86 | |
No blank line here. The "# personal repos" should go on the next line. General comment about comments: They should … |
chipx86 | |
Maybe add a body to this docstring, talking about how this is used to represent a configured repository already on … |
chipx86 | |
Our general form for test names will be like: "test_<funcname>with<conditions>" Same below. |
chipx86 | |
All the docstrings should be in the form of "Testing GitHub.get_remote_repositories with ..." This helps us identify things when tests … |
chipx86 | |
Indented too far, and the {} shouldn't be grouped with the []. Should be like: repos = [ { 'id': … |
chipx86 | |
Same comments here about our preferred syntax for things. Also the ones below. |
chipx86 | |
There's a trailing space before the """ |
chipx86 | |
Blank line before this. |
chipx86 | |
You should be able to use api_get instead of http_get, which will also do the json_loads and handle errors in … |
chipx86 | |
And before this. |
chipx86 | |
And between these. |
chipx86 | |
Summary must be on the same line as the """. |
chipx86 | |
This regex should be pulled out and compiled into a regex on the class. |
chipx86 | |
'next' is a reserved word in Python. |
chipx86 | |
Blank line between these. |
chipx86 | |
No blank line. |
chipx86 | |
What is type? Can we make this a more descriptive name? |
chipx86 | |
This shouldn't be a classmethod. |
chipx86 | |
Needs to be 2 blank lines. |
chipx86 | |
No trailing period. Same below. |
chipx86 | |
Shouldn't use the u prefix for strings. Also, we prefer single quotes for strings whenever possible. Same below. |
chipx86 | |
Can't this be false? Same below. |
chipx86 | |
true? |
chipx86 | |
Blank line before conditionals. |
chipx86 | |
next is a reserved word. Maybe just put this inline below. |
chipx86 | |
No trailing period. Same below. |
chipx86 | |
This should use self.next_link |
david | |
The first line of this file should be: from __future__ import unicode_literals |
david |
-
This looks great.
All my comments are about the style we like for code going into the product. This is the subjective stuff, which means there's not really a "right or wrong," but we like consistency :)
-
-
No blank line here.
The "# personal repos" should go on the next line.
General comment about comments: They should generally be proper sentences, with sentence caps and (when appropriate) trailing periods.
-
Maybe add a body to this docstring, talking about how this is used to represent a configured repository already on the hosting service, and does not necessarily match a repository configured on Review Board.
-
-
All the docstrings should be in the form of "Testing GitHub.get_remote_repositories with ..."
This helps us identify things when tests fail.
Also, as a special exception to the docstring rules, if the docstring reaches beyond the 79th column, you can wrap to the next line. This isn't typically allowed for unit test (it'll cut things off), but we use some custom stuff to allow it.
(I'd also change "repos" to "repositories", given that. Same below.)
-
Indented too far, and the {} shouldn't be grouped with the []. Should be like:
repos = [ { 'id': ..., ... }, { ... } ]
You also shouldn't have the
u
prefix in front of strings.We also like single-quote strings instead of double-quote, and we like trailing commas on the last item in any list or dictionary.
-
- Groups:
-
You still have some open issues from previous reviews. Can you verify whether those are fixed, and close them if so?
I also see a little bit of code duplication between this and /r/5602/. What depends on what here?
-
-
-
Thanks Olesssia! I've made some changes to get this working again (we had some refactoring of the GitHub code that broke your change), and made a few other small organizational changes. Otherwise, very much intact, and we'll be using this as the base for new work going into (hopefully) 2.1!
Going to close this in favor of the updated change, since we don't have the ability to transfer ownership yet.