Add support of supports_post_commit for Mercurial

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

Information

Review Board
release-2.5.x
af463fb...

Reviewers

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.

Description From Last Updated

j before l

daviddavid

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

daviddavid

Needs a docstring.

daviddavid

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

daviddavid

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

daviddavid

Needs a docstring.

daviddavid

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

daviddavid

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

daviddavid

Needs a docstring.

daviddavid

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

daviddavid

Comments should be in sentence case with punctuation. Also, I can't tell if this applies to the code below, or …

daviddavid

Needs a docstring.

daviddavid

This shouldn't be necessary.

daviddavid

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

daviddavid

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

daviddavid

This isn't a particularly useful error.

daviddavid

Needs a docstring.

daviddavid

Indentation is weird here (see above)

daviddavid

Needs a docstring.

daviddavid

Needs a docstring.

daviddavid

Needs a docstring.

daviddavid

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

daviddavid

This isn't a particularly useful error message.

daviddavid

This isn't a particularly useful error message.

daviddavid

I'd prefer not to rename a standard package like this. We also typically list full-package imports before from ... imports.

daviddavid

Please use single quotes here.

daviddavid

Can you format this so there's a one-line summary, then a blank line, then a longer description (or just a …

daviddavid

Please use single quotes here.

daviddavid

Same here re: docstring formatting.

daviddavid

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

daviddavid

Please use single quotes here.

daviddavid

Same re: docstring formatting.

daviddavid

Please use single quotes here.

daviddavid

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

daviddavid

If the process fails, this should raise an SCMError.

daviddavid

Same re: docstring formatting.

daviddavid

If the process fails, this should raise an SCMError.

daviddavid

Same re: docstring formatting.

daviddavid

Please use single quotes here.

daviddavid

Please use single quotes here. Also please include the revision in the error output. We should also raise an SCMError

daviddavid

Please don't put these all on different lines.

brenniebrennie

Does this return an ISO 8601 date or parse an ISO 8601 date? Also, needs an args and return block: …

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

This should be indented like: results.append(Branch( id=data['branch'], ...)

brenniebrennie

Needs Args and Returns.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

Needs Args and Returns

brenniebrennie

blank line between these.

brenniebrennie

blank line between these.

brenniebrennie

blank line between these.

brenniebrennie

blank line between these.

brenniebrennie

Args and Returns

brenniebrennie

Blank line between these.

brenniebrennie

You should pass interpolation args as args to error, e.g. logging.error('foo %s', bar)

brenniebrennie

Why is this a string?

brenniebrennie

Missing returns

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

See above comment about indentation.

brenniebrennie

Args and Returns

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

See above comment about indentation.

brenniebrennie

Args and returns.

brenniebrennie

BLank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

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

daviddavid

Should be formatted as such: Returns: list of reviewboard.scmtools.core.Branch: A list of the active branches.

daviddavid

Should be formatted as: Args: revision (str): Identifier of changeset. branch (str): An optional branch name to filter by. Returns: …

daviddavid

Please add Args/Returns.

daviddavid

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

daviddavid

Description should go on the next line.

daviddavid

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

daviddavid

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

daviddavid

Add a period at the end of this sentence.

daviddavid

Add a blank line in here.

daviddavid

Add a period at the end of this sentence.

daviddavid

Add a period at the end of this sentence.

daviddavid

Add a period at the end of this sentence.

daviddavid

Add a period at the end of this sentence.

daviddavid

Col: 12 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 12 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 31 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 31 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 31 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 31 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 31 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (93 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (93 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (87 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

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

brenniebrennie

Docstrings.

brenniebrennie

"date" ? why is this in quotes?

brenniebrennie

unicode.

brenniebrennie

See other comments about docstrings

brenniebrennie

See other comments about docstrings

brenniebrennie

unicode.

brenniebrennie

can you add the exception to this, e.g. logging.exception('Cannot load details of changeset from hgweb: %s', e)

brenniebrennie

See other comment about your docstrings.

brenniebrennie

unicode

brenniebrennie

Blank line between these.

brenniebrennie

unicode

brenniebrennie

"Return detailed..."

brenniebrennie

Can you improve this, e.g. This method retrieves the patch in JSON format from hgweb or similar.

brenniebrennie

unicode

brenniebrennie

"Return" instead of "Get"

brenniebrennie

This should mention it runs hg and parses the result out, because the method name does not indicate that it …

brenniebrennie

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

brenniebrennie

list of unicode

brenniebrennie

commit

brenniebrennie

Can you reformat this last line as: parent=p, base_commit_id=p ))

brenniebrennie

unicode

brenniebrennie

Blank line between these.

brenniebrennie

unicode

brenniebrennie

commit

brenniebrennie

blank line between these.

brenniebrennie

'-r%s:0' % start

brenniebrennie

Reformat as: cmd = [ 'log', revset, # ..., ]

brenniebrennie

Blank line between these.

brenniebrennie

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

brenniebrennie

See other comments about docstrings

brenniebrennie

unicode.

brenniebrennie

Shouldn't this take revision not cmd?

brenniebrennie

Incorrect docstring.

brenniebrennie

Can we sort these alphabetically?

daviddavid

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

daviddavid

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

daviddavid

Maybe call this date_tuple_to_iso8601?

daviddavid

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

daviddavid

This whole thing could be a single list comprehension: results = [ Branch( id=data['branch'] commit=data['node'], default=(data['branch'] == 'default')) for data …

daviddavid

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

daviddavid

unicode, optional

daviddavid

unicode, optional

daviddavid

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

daviddavid

query should be urlencoded here, no?

daviddavid

Should revision be URL-encoded?

daviddavid

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

daviddavid

Same here.

daviddavid

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

daviddavid

This can all be a list comprehension.

daviddavid

Same here with being explicit in the comparison.

daviddavid

unicode, optional

daviddavid

unicode, optional

daviddavid

This returns a list of Commit, no?

daviddavid

Please compare explicitly against 0.

daviddavid

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

daviddavid

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

daviddavid

I don't think we should be decoding this. The diff should stay as bytes so that it will work correctly …

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