HostingService: remote repositories for user

Review Request #5432 — Created Feb. 8, 2014 and discarded

Information

Review Board
master

Reviewers

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.

chipx86chipx86

When there's only a summary, the trailing """ should be on the first line.

chipx86chipx86

I think we should be explicit about what we pull out. As discussed, we'll also need to transform url to …

chipx86chipx86

Docstrings are always in one of the following two forms: """One-line summary.""" or: """One-line summary Multi-line description. """

chipx86chipx86

As discussed, hould be path and mirror_path.

chipx86chipx86

I think any extra data should be explicitly provided in an extra_data field in the constructor, and stored in a …

chipx86chipx86

Same comment above about the docstring. Documentation should always be clear about what's being returned. Instead of "maybe instances," the …

chipx86chipx86

Same comment about import order.

chipx86chipx86

This function needs a docstring matching the format of the previous docstrings.

chipx86chipx86

This looks indented too far. Also, the trailing brace should match the beginning of the line opening the brace (so, …

chipx86chipx86

This should specifically compare the fields we care about.

chipx86chipx86

There should still be two blank lines here.

chipx86chipx86

No blank line here. The "# personal repos" should go on the next line. General comment about comments: They should …

chipx86chipx86

Maybe add a body to this docstring, talking about how this is used to represent a configured repository already on …

chipx86chipx86

Our general form for test names will be like: "test_<funcname>with<conditions>" Same below.

chipx86chipx86

All the docstrings should be in the form of "Testing GitHub.get_remote_repositories with ..." This helps us identify things when tests …

chipx86chipx86

Indented too far, and the {} shouldn't be grouped with the []. Should be like: repos = [ { 'id': …

chipx86chipx86

Same comments here about our preferred syntax for things. Also the ones below.

chipx86chipx86

There's a trailing space before the """

chipx86chipx86

Blank line before this.

chipx86chipx86

You should be able to use api_get instead of http_get, which will also do the json_loads and handle errors in …

chipx86chipx86

And before this.

chipx86chipx86

And between these.

chipx86chipx86

Summary must be on the same line as the """.

chipx86chipx86

This regex should be pulled out and compiled into a regex on the class.

chipx86chipx86

'next' is a reserved word in Python.

chipx86chipx86

Blank line between these.

chipx86chipx86

No blank line.

chipx86chipx86

What is type? Can we make this a more descriptive name?

chipx86chipx86

This shouldn't be a classmethod.

chipx86chipx86

Needs to be 2 blank lines.

chipx86chipx86

No trailing period. Same below.

chipx86chipx86

Shouldn't use the u prefix for strings. Also, we prefer single quotes for strings whenever possible. Same below.

chipx86chipx86

Can't this be false? Same below.

chipx86chipx86

true?

chipx86chipx86

Blank line before conditionals.

chipx86chipx86

next is a reserved word. Maybe just put this inline below.

chipx86chipx86

No trailing period. Same below.

chipx86chipx86

This should use self.next_link

daviddavid

The first line of this file should be: from __future__ import unicode_literals

daviddavid
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues

    This should be included above with the other reviewboard imports, listed alphabetically.

  3. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues

    When there's only a summary, the trailing """ should be on the first line.

  4. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues

    I think we should be explicit about what we pull out. As discussed, we'll also need to transform url to path and stuff.

  5. reviewboard/hostingsvcs/hsrepositoty.py (Diff revision 1)
     
     
     
     
    Show all issues

    Docstrings are always in one of the following two forms:

    """One-line summary."""
    

    or:

    """One-line summary
    
    Multi-line description.
    """
    
  6. reviewboard/hostingsvcs/hsrepositoty.py (Diff revision 1)
     
     
     
    Show all issues

    As discussed, hould be path and mirror_path.

  7. reviewboard/hostingsvcs/hsrepositoty.py (Diff revision 1)
     
     
     
    Show all issues

    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.

  8. reviewboard/hostingsvcs/service.py (Diff revision 1)
     
     
     
    Show all issues

    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."

  9. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
    Show all issues

    Same comment about import order.

  10. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
    Show all issues

    This function needs a docstring matching the format of the previous docstrings.

  11. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This looks indented too far.

    Also, the trailing brace should match the beginning of the line opening the brace (so, same level as repos)

  12. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
    Show all issues

    This should specifically compare the fields we care about.

  13. 
      
OL
chipx86
  1. 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 :)

  2. reviewboard/hostingsvcs/github.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    There should still be two blank lines here.

  3. reviewboard/hostingsvcs/github.py (Diff revision 2)
     
     
     
     
    Show all issues

    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.

  4. reviewboard/hostingsvcs/repository.py (Diff revision 2)
     
     
    Show all issues

    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.

  5. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues

    Our general form for test names will be like: "test_<funcname>with<conditions>"

    Same below.

  6. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues

    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.)

  7. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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.

  8. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Same comments here about our preferred syntax for things.

    Also the ones below.

  9. 
      
OL
OL
OL
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
    Show all issues

    There's a trailing space before the """

  3. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
    Show all issues

    Blank line before this.

  4. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
     
    Show all issues

    You should be able to use api_get instead of http_get, which will also do the json_loads and handle errors in the payload.

  5. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
    Show all issues

    And before this.

  6. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
     
    Show all issues

    And between these.

  7. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
     
    Show all issues

    Summary must be on the same line as the """.

  8. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
    Show all issues

    This regex should be pulled out and compiled into a regex on the class.

  9. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
    Show all issues

    'next' is a reserved word in Python.

  10. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
     
    Show all issues

    Blank line between these.

  11. reviewboard/hostingsvcs/repository.py (Diff revision 4)
     
     
     
     
    Show all issues

    No blank line.

  12. reviewboard/hostingsvcs/repository.py (Diff revision 4)
     
     
    Show all issues

    What is type? Can we make this a more descriptive name?

  13. reviewboard/hostingsvcs/service.py (Diff revision 4)
     
     
     
    Show all issues

    This shouldn't be a classmethod.

  14. reviewboard/hostingsvcs/tests.py (Diff revision 4)
     
     
     
     
     
    Show all issues

    Needs to be 2 blank lines.

  15. reviewboard/hostingsvcs/tests.py (Diff revision 4)
     
     
    Show all issues

    No trailing period.

    Same below.

  16. reviewboard/hostingsvcs/tests.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
    Show all issues

    Shouldn't use the u prefix for strings.

    Also, we prefer single quotes for strings whenever possible.

    Same below.

  17. reviewboard/hostingsvcs/tests.py (Diff revision 4)
     
     
    Show all issues

    Can't this be false?

    Same below.

  18. reviewboard/hostingsvcs/tests.py (Diff revision 4)
     
     
    Show all issues

    true?

  19. reviewboard/hostingsvcs/tests.py (Diff revision 4)
     
     
    Show all issues

    Blank line before conditionals.

  20. reviewboard/hostingsvcs/tests.py (Diff revision 4)
     
     
    Show all issues

    next is a reserved word. Maybe just put this inline below.

  21. reviewboard/hostingsvcs/tests.py (Diff revision 4)
     
     
    Show all issues

    No trailing period.

    Same below.

  22. 
      
OL
david
  1. 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?

    1. 5602 depends on this, sorry I was somewhat confused about how to create branches and have access to the code but not post changes twice..

  2. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    This should use self.next_link

  3. reviewboard/hostingsvcs/repository.py (Diff revision 5)
     
     
    Show all issues

    The first line of this file should be:

    from __future__ import unicode_literals
    
  4. 
      
OL
chipx86
  1. 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.

  2. 
      
OL
Review request changed
Status:
Discarded
Change Summary:
Made some changes and posted as /r/5867, which replaces this change.