Add support of supports_post_commit for Mercurial

Review Request #7917 — Created Jan. 29, 2016 and submitted

misery
Review Board
release-2.5.x
4032
8626
af463fb...
reviewboard

This change adds the "post review" feature for mercurial
repositories. So it is possible to select changesets
with "New Review Request" page.

Remote repositories are supported by hgweb json
interface. Be aware that Mercurial 3.9 is required.

Local repositories are supported by json templates
interface via cmdline --template json.

See: https://selenic.com/repo/hg/help/hgweb

Bugs closed: 4032

HgWebClient:
Used "hg serve" for remote repository!

HgLocalClient:
Used local (directory) repository!

Switched between branches, scrolled list to get more changesets and created different reviews from changesets.

  • 0
  • 0
  • 154
  • 5
  • 159
Description From Last Updated
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/hg.py
    
    
  2. 
      
misery
  1. I have a problem that the initial list is empty. If I switch to another branch to switch back to 'default' the list will be shown. I don't know how to fix this! Any hints?

    I'm new in python and RB code. So I don't know when to throw an exception and when to return empty values. Please review this! :-)

    1. Ok, looks like it was a cache problem. List is immediately filled with changesets now.

      By the way.... I think there is a mistake with datetime as it shows the wrong time. There is a problem with the timezone from RB and Mercurial. Any hints?

  2. 
      
david
  1. 
      
  2. reviewboard/scmtools/hg.py (Diff revision 1)
     
     
     

    j before l

  3. reviewboard/scmtools/hg.py (Diff revision 1)
     
     
     
     

    Can we change this to surround imports with parens instead of using characters?

  4. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Needs a docstring.

  5. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Can we make this use logging.exception, and flesh out the log message a bit to include some mention of mercurial?

  6. reviewboard/scmtools/hg.py (Diff revision 1)
     
     
     
     

    Indentation is weird here. How about:

    for data in json.loads(contents)['branches']:
        if data['status'] != 'closed':
            results.append(Branch(
                id=data['branch'],
                commit=data['node'],
                default=(data['branch'] == 'default')))
    
  7. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Needs a docstring.

  8. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Can we make this use logging.exception, and flesh out the log message a bit to include some mention of mercurial?

  9. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Can we call this "parent" instead of "p"?

  10. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Needs a docstring.

  11. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Can we make this use logging.exception, and flesh out the log message a bit to include some mention of mercurial?

  12. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Comments should be in sentence case with punctuation. Also, I can't tell if this applies to the code below, or is some kind of TODO comment. Can you clarify?

  13. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Needs a docstring.

  14. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    This shouldn't be necessary.

  15. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Can we make this use logging.exception, and flesh out the log message a bit to include some mention of mercurial?

  16. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Better as contents.decode('utf-8')

  17. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    This isn't a particularly useful error.

  18. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Needs a docstring.

  19. reviewboard/scmtools/hg.py (Diff revision 1)
     
     
     
     

    Indentation is weird here (see above)

  20. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Needs a docstring.

  21. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Needs a docstring.

  22. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Needs a docstring.

  23. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    list is a python reserved name. Please change this variable name.

  24. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    This isn't a particularly useful error message.

  25. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    This isn't a particularly useful error message.

  26. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/hg.py
    
    
  2. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/hg.py
    
    
  2. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/hg.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/scmtools/hg.py (Diff revision 4)
     
     

    I'd prefer not to rename a standard package like this.

    We also typically list full-package imports before from ... imports.

  3. reviewboard/scmtools/hg.py (Diff revision 4)
     
     

    Please use single quotes here.

  4. reviewboard/scmtools/hg.py (Diff revision 4)
     
     
     

    Can you format this so there's a one-line summary, then a blank line, then a longer description (or just a one-line summary that doesn't have to wrap)?

  5. reviewboard/scmtools/hg.py (Diff revision 4)
     
     

    Please use single quotes here.

  6. reviewboard/scmtools/hg.py (Diff revision 4)
     
     
     

    Same here re: docstring formatting.

  7. reviewboard/scmtools/hg.py (Diff revision 4)
     
     

    We use self.path.rstrip('/') a lot. Can we save that somewhere?

  8. reviewboard/scmtools/hg.py (Diff revision 4)
     
     

    Please use single quotes here.

  9. reviewboard/scmtools/hg.py (Diff revision 4)
     
     
     

    Same re: docstring formatting.

  10. reviewboard/scmtools/hg.py (Diff revision 4)
     
     

    Please use single quotes here.

  11. reviewboard/scmtools/hg.py (Diff revision 4)
     
     

    Please use single quotes here. Also, is there a more specific error that can be logged? This should probably at least include the revision.

  12. reviewboard/scmtools/hg.py (Diff revision 4)
     
     

    If the process fails, this should raise an SCMError.

  13. reviewboard/scmtools/hg.py (Diff revision 4)
     
     
     

    Same re: docstring formatting.

  14. reviewboard/scmtools/hg.py (Diff revision 4)
     
     

    If the process fails, this should raise an SCMError.

  15. reviewboard/scmtools/hg.py (Diff revision 4)
     
     
     

    Same re: docstring formatting.

  16. reviewboard/scmtools/hg.py (Diff revision 4)
     
     

    Please use single quotes here.

  17. reviewboard/scmtools/hg.py (Diff revision 4)
     
     

    Please use single quotes here. Also please include the revision in the error output.

    We should also raise an SCMError

  18. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/hg.py
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     
     
     
     
     
     
     

    Please don't put these all on different lines.

  3. reviewboard/scmtools/hg.py (Diff revision 5)
     
     

    Does this return an ISO 8601 date or parse an ISO 8601 date?

    Also, needs an args and return block:

    ```
    """

    Args:
    data (type_of_data):
    Description of data

    Returns:
    return_type:
    Description of return value.
    """

  4. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     

    Blank line between these.

  5. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     

    Blank line between these.

  6. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     
     
     

    This should be indented like:

    results.append(Branch(
        id=data['branch'],
        ...)
    
  7. reviewboard/scmtools/hg.py (Diff revision 5)
     
     

    Needs Args and Returns.

  8. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     

    Blank line between these.

  9. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     

    Blank line between these.

  10. reviewboard/scmtools/hg.py (Diff revision 5)
     
     

    Needs Args and Returns

  11. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     

    blank line between these.

  12. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     

    blank line between these.

  13. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     

    blank line between these.

  14. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     

    blank line between these.

  15. reviewboard/scmtools/hg.py (Diff revision 5)
     
     

    Args and Returns

  16. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     

    Blank line between these.

  17. reviewboard/scmtools/hg.py (Diff revision 5)
     
     

    You should pass interpolation args as args to error, e.g.

    logging.error('foo %s', bar)

  18. reviewboard/scmtools/hg.py (Diff revision 5)
     
     

    Missing returns

  19. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     

    Blank line between these.

  20. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     

    Blank line between these.

  21. reviewboard/scmtools/hg.py (Diff revision 5)
     
     

    See above comment about indentation.

  22. reviewboard/scmtools/hg.py (Diff revision 5)
     
     

    Args and Returns

  23. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     

    Blank line between these.

  24. reviewboard/scmtools/hg.py (Diff revision 5)
     
     

    Blank line between these.

  25. reviewboard/scmtools/hg.py (Diff revision 5)
     
     

    See above comment about indentation.

  26. reviewboard/scmtools/hg.py (Diff revision 5)
     
     

    Args and returns.

  27. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     

    BLank line between these.

  28. reviewboard/scmtools/hg.py (Diff revision 5)
     
     
     

    Blank line between these.

  29. 
      
brennie
  1. 
      
  2. reviewboard/scmtools/hg.py (Diff revision 5)
     
     

    Why is this a string?

    1. Because it is used as str only to have "page for changesets" like subversion scmtools does.

  3. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/hg.py
    
    
  2. 
      
david
  1. The docstrings for a lot of this don't match the format that our documentation tools expect. Please see https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/ for details. I've marked the first few but I didn't go all the way through.

  2. reviewboard/scmtools/hg.py (Diff revision 6)
     
     
     
     

    The format here isn't quite right, I think. It looks like the contents of data are ints, not strings. This should therefore be:

    Args:
        data (tuple of int):
            A 2-tuple, where the first item is a unix timestamp
            and the second is the timezone offset.
    
  3. reviewboard/scmtools/hg.py (Diff revision 6)
     
     

    Should be formatted as such:

    Returns:
        list of reviewboard.scmtools.core.Branch:
        A list of the active branches.
    
  4. reviewboard/scmtools/hg.py (Diff revision 6)
     
     
     

    Should be formatted as:

    Args:
        revision (str):
            Identifier of changeset.
    
        branch (str):
            An optional branch name to filter by.
    
    Returns:
        reviewboard.scmtools.core.Commit:
        The commit object.
    
  5. reviewboard/scmtools/hg.py (Diff revision 6)
     
     

    Please add Args/Returns.

    1. Why do I need to add parameter documentation for an overriden method? It just implements the interface of SCMClient. That would be a duplicated documentation as it should be documented there.

  6. reviewboard/scmtools/hg.py (Diff revision 6)
     
     

    Description should go on the next line, indented 4 spaces.

  7. reviewboard/scmtools/hg.py (Diff revision 6)
     
     

    Description should go on the next line.

  8. reviewboard/scmtools/hg.py (Diff revision 6)
     
     

    Should have "list of reviewboard.scmtools.core.Branch" and the description on the next line.

  9. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/hg.py
    
    
  2. 
      
david
  1. Looking very good! Just a few tiny style points left.

  2. reviewboard/scmtools/hg.py (Diff revision 7)
     
     

    Can you put parens around the conditional? That way it's a little more readable about what the parameter value is.

  3. reviewboard/scmtools/hg.py (Diff revision 7)
     
     

    Add a period at the end of this sentence.

  4. reviewboard/scmtools/hg.py (Diff revision 7)
     
     
     

    Add a blank line in here.

  5. reviewboard/scmtools/hg.py (Diff revision 7)
     
     

    Add a period at the end of this sentence.

  6. reviewboard/scmtools/hg.py (Diff revision 7)
     
     

    Add a period at the end of this sentence.

  7. reviewboard/scmtools/hg.py (Diff revision 7)
     
     

    Add a period at the end of this sentence.

  8. reviewboard/scmtools/hg.py (Diff revision 7)
     
     

    Add a period at the end of this sentence.

  9. 
      
chipx86
  1. A couple higher-level things I'd like to see before this goes in:

    1) Can you update your review request description to go into more detail about what this is doing and how it works? See https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/ for what we like to see.
    2) Can you add unit tests that simulate the responses from the server, and verify the different combinations of arguments to get_branches and such? Should also test any error conditions. This will help ensure that this is actually working as expected, and does not regress in the future (and gives us a better compatibility matrix to test against if hgweb and such should ever change).

  2. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
  2. reviewboard/scmtools/hg.py (Diff revision 8)
     
     
    Col: 12
     E111 indentation is not a multiple of four
    
  3. reviewboard/scmtools/hg.py (Diff revision 8)
     
     
    Col: 12
     E111 indentation is not a multiple of four
    
  4. reviewboard/scmtools/hg.py (Diff revision 8)
     
     
    Col: 31
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/scmtools/hg.py (Diff revision 8)
     
     
    Col: 31
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/scmtools/hg.py (Diff revision 8)
     
     
    Col: 31
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/scmtools/hg.py (Diff revision 8)
     
     
    Col: 31
     E128 continuation line under-indented for visual indent
    
  8. reviewboard/scmtools/hg.py (Diff revision 8)
     
     
    Col: 31
     E128 continuation line under-indented for visual indent
    
  9. reviewboard/scmtools/tests/test_hg.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  10. reviewboard/scmtools/tests/test_hg.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  11. reviewboard/scmtools/tests/test_hg.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  12. reviewboard/scmtools/tests/test_hg.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  13. reviewboard/scmtools/tests/test_hg.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (93 > 79 characters)
    
  14. reviewboard/scmtools/tests/test_hg.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  15. reviewboard/scmtools/tests/test_hg.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  16. reviewboard/scmtools/tests/test_hg.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  17. reviewboard/scmtools/tests/test_hg.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (93 > 79 characters)
    
  18. reviewboard/scmtools/tests/test_hg.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  19. reviewboard/scmtools/tests/test_hg.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (87 > 79 characters)
    
  20. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
  2. reviewboard/scmtools/hg.py (Diff revision 9)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  3. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    This should be the last import in the group. from foo import bar goes after import foo.

  3. reviewboard/scmtools/hg.py (Diff revision 10)
     
     
     
     
     
     
     
     
     

    Docstrings.

  4. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    "date" ? why is this in quotes?

  5. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    unicode.

  6. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    See other comments about docstrings

  7. reviewboard/scmtools/hg.py (Diff revision 10)
     
     
     
     

    See other comments about docstrings

  8. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    unicode.

  9. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    can you add the exception to this, e.g.

    logging.exception('Cannot load details of changeset from hgweb: %s', e)
    
    1. logging.exception should already print it, right?!?

    2. Technically, yes, but separately from this log entry, at the end of a traceback. This makes it much harder to spot the actual error, particularly when grepping logs or using log management software with filters. We always display the exception message in the log message.

  10. reviewboard/scmtools/hg.py (Diff revision 10)
     
     
     
     

    See other comment about your docstrings.

  11. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    unicode

  12. reviewboard/scmtools/hg.py (Diff revision 10)
     
     
     

    Blank line between these.

  13. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    unicode

  14. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    "Return detailed..."

  15. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    Can you improve this, e.g.

    This method retrieves the patch in JSON format from hgweb or similar.

  16. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    unicode

  17. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    "Return" instead of "Get"

  18. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    This should mention it runs hg and parses the result out, because the method name does not indicate that it would take a command. I would expect this to take a revision.

    Instead of taking a command, this should take the revision range and generate the command for it.

    1. Take a revision makes is more complicated because of "branch" and "commits page limit". I added more documentation and pass a revset instead of a cmd.

  19. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    This should be a phrase in the imperitive mood. The first word should be a verb.

  20. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    list of unicode

  21. reviewboard/scmtools/hg.py (Diff revision 10)
     
     
  22. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    Can you reformat this last line as:

       parent=p,
       base_commit_id=p
    ))
    
  23. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    unicode

  24. reviewboard/scmtools/hg.py (Diff revision 10)
     
     
     

    Blank line between these.

  25. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    unicode

  26. reviewboard/scmtools/hg.py (Diff revision 10)
     
     
  27. reviewboard/scmtools/hg.py (Diff revision 10)
     
     
     

    blank line between these.

  28. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    '-r%s:0' % start

  29. reviewboard/scmtools/hg.py (Diff revision 10)
     
     
     
     

    Reformat as:

    cmd = [
        'log', revset, # ...,
    ]
    
  30. reviewboard/scmtools/hg.py (Diff revision 10)
     
     
     

    Blank line between these.

  31. reviewboard/scmtools/hg.py (Diff revision 10)
     
     
     

    You can replace this with: cmd.extend(('-b', branch)).

  32. reviewboard/scmtools/hg.py (Diff revision 10)
     
     
     
     

    See other comments about docstrings

  33. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    unicode.

  34. reviewboard/scmtools/hg.py (Diff revision 10)
     
     

    Shouldn't this take revision not cmd?

  35. reviewboard/scmtools/tests/test_hg.py (Diff revision 10)
     
     
     

    Incorrect docstring.

  36. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
  2. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
  2. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
  2. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
  2. 
      
misery
  1. 
      
  2. Are there any issues? I like to push it before first anniversary. :-)
  3. 
      
david
  1. 
      
  2. reviewboard/scmtools/hg.py (Diff revision 14)
     
     
     
     

    Can we sort these alphabetically?

  3. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    Should be unicode, optional, since there's a default.

  4. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    Should be unicode, optional, since there's a default.

  5. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    Maybe call this date_tuple_to_iso8601?

  6. reviewboard/scmtools/hg.py (Diff revision 14)
     
     
     
     

    If you move the results assignment to the top, you could have this be return results

  7. reviewboard/scmtools/hg.py (Diff revision 14)
     
     
     
     
     
     
     

    This whole thing could be a single list comprehension:

    results = [
        Branch(
            id=data['branch']
            commit=data['node'],
            default=(data['branch'] == 'default'))
        for data in json.loads(contents)['branches']
        if data['status'] != 'closed'
    ]
    
  8. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    Can the revision include any special characters in it? Should revision be URL-encoded?

    1. No, it contains alpha-numeric only. Also the special syntax () and + needs to be passed "as is".

  9. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    unicode, optional

  10. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    unicode, optional

  11. reviewboard/scmtools/hg.py (Diff revision 14)
     
     
     
     
     
     
     
     
     
     

    This kind of query concatenation is a little error-prone for future changes. Can we instead do something like this?

    query_parts = []
    
    if start:
        query_parts.append('ancestors(%s)' % start)
    
    query_parts.append('branch(%s)' % (branch or '.'))
    
    query = '+and+'.join(query_parts)
    
  12. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    query should be urlencoded here, no?

  13. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    Should revision be URL-encoded?

  14. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    Returning an empty list here seems wrong. I think it's more correct to raise an SCMError.

  15. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    Same here.

  16. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    Checking truthiness here isn't really clear. How about if p.wait() != 0:?

  17. reviewboard/scmtools/hg.py (Diff revision 14)
     
     
     
     
     
     
     
     
     

    This can all be a list comprehension.

  18. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    Same here with being explicit in the comparison.

  19. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    unicode, optional

  20. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    unicode, optional

  21. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    This returns a list of Commit, no?

  22. reviewboard/scmtools/hg.py (Diff revision 14)
     
     

    Please compare explicitly against 0.

  23. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/scmtools/hg.py (Diff revision 15)
     
     

    This line shouldn't be necessary. We don't actually use contents until after the try/except block in which it's assigned.

  3. reviewboard/scmtools/hg.py (Diff revision 15)
     
     
     

    When would changeset be falsy? Can't we just always append it?

  4. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/scmtools/hg.py (Diff revision 16)
     
     

    I don't think we should be decoding this. The diff should stay as bytes so that it will work correctly if the file is non-UTF8.

  3. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/tests/test_hg.py
        reviewboard/scmtools/hg.py
    
    
  2. 
      
david
  1. The code for this is looking pretty good, but I'd like to see some docs for this (either in this change or a separate one). The change description here makes it sound like there are certain limitations and potentially complex configuration.

    1. Well.... where is the destination for that? There is no complex configuration. Reviewboard already requires hgweb for remote repo and local repos works out of the box.

    2. https://reviews.reviewboard.org/r/8626/

    3. ping?

    4. We're currently locked down for some releases, but I plan to get to this once those are out the door.

  2. 
      
david
  1. Ship It!
  2. 
      
misery
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (8addf86)
Loading...