Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/hg.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/hg.py

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! :-)

j before l

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

Needs a docstring.

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

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

Needs a docstring.

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

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

Needs a docstring.

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

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?

Needs a docstring.

This shouldn't be necessary.

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

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

This isn't a particularly useful error.

Needs a docstring.

Indentation is weird here (see above)

Needs a docstring.

Needs a docstring.

Needs a docstring.

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

This isn't a particularly useful error message.

This isn't a particularly useful error message.

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/hg.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/hg.py

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/hg.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/hg.py

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/hg.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/hg.py

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

Please use single quotes here.

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

Please use single quotes here.

Same here re: docstring formatting.

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

Please use single quotes here.

Same re: docstring formatting.

Please use single quotes here.

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

If the process fails, this should raise an SCMError.

Same re: docstring formatting.

If the process fails, this should raise an SCMError.

Same re: docstring formatting.

Please use single quotes here.

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

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/hg.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/hg.py

Please don't put these all on different lines.

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.

"""

Blank line between these.

Blank line between these.

This should be indented like:

results.append(Branch(
    id=data['branch'],
    ...)

Needs Args and Returns.

Blank line between these.

Blank line between these.

Needs Args and Returns

blank line between these.

blank line between these.

blank line between these.

blank line between these.

Args and Returns

Blank line between these.

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

Missing returns

Blank line between these.

Blank line between these.

See above comment about indentation.

Args and Returns

Blank line between these.

Blank line between these.

See above comment about indentation.

Args and returns.

BLank line between these.

Blank line between these.

Why is this a string?

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/hg.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/hg.py

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.

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.

Should be formatted as such:

Returns:
    list of reviewboard.scmtools.core.Branch:
    A list of the active branches.

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.

Please add Args/Returns.

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

Description should go on the next line.

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

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/hg.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/hg.py

Looking very good! Just a few tiny style points left.

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

Add a period at the end of this sentence.

Add a blank line in here.

Add a period at the end of this sentence.

Add a period at the end of this sentence.

Add a period at the end of this sentence.

Add a period at the end of this sentence.

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

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

Col: 12
 E111 indentation is not a multiple of four

Col: 12
 E111 indentation is not a multiple of four

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/tests/test_hg.py
    reviewboard/scmtools/hg.py

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

Docstrings.

"date" ? why is this in quotes?

unicode.

See other comments about docstrings

See other comments about docstrings

unicode.

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

logging.exception('Cannot load details of changeset from hgweb: %s', e)

See other comment about your docstrings.

unicode

Blank line between these.

unicode

"Return detailed..."

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

unicode

"Return" instead of "Get"

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.

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

list of unicode

commit

Can you reformat this last line as:

   parent=p,
   base_commit_id=p
))

unicode

Blank line between these.

unicode

commit

blank line between these.

'-r%s:0' % start

Reformat as:

cmd = [
    'log', revset, # ...,
]

Blank line between these.

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

See other comments about docstrings

unicode.

Shouldn't this take revision not cmd?

Incorrect docstring.