• 
      

    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.