Add support of supports_post_commit for Mercurial
Review Request #7917 — Created Jan. 29, 2016 and submitted
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 |
david | |
Can we change this to surround imports with parens instead of using characters? |
david | |
Needs a docstring. |
david | |
Can we make this use logging.exception, and flesh out the log message a bit to include some mention of mercurial? |
david | |
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'))) |
david | |
Needs a docstring. |
david | |
Can we make this use logging.exception, and flesh out the log message a bit to include some mention of mercurial? |
david | |
Can we call this "parent" instead of "p"? |
david | |
Needs a docstring. |
david | |
Can we make this use logging.exception, and flesh out the log message a bit to include some mention of mercurial? |
david | |
Comments should be in sentence case with punctuation. Also, I can't tell if this applies to the code below, or … |
david | |
Needs a docstring. |
david | |
This shouldn't be necessary. |
david | |
Can we make this use logging.exception, and flesh out the log message a bit to include some mention of mercurial? |
david | |
Better as contents.decode('utf-8') |
david | |
This isn't a particularly useful error. |
david | |
Needs a docstring. |
david | |
Indentation is weird here (see above) |
david | |
Needs a docstring. |
david | |
Needs a docstring. |
david | |
Needs a docstring. |
david | |
list is a python reserved name. Please change this variable name. |
david | |
This isn't a particularly useful error message. |
david | |
This isn't a particularly useful error message. |
david | |
I'd prefer not to rename a standard package like this. We also typically list full-package imports before from ... imports. |
david | |
Please use single quotes here. |
david | |
Can you format this so there's a one-line summary, then a blank line, then a longer description (or just a … |
david | |
Please use single quotes here. |
david | |
Same here re: docstring formatting. |
david | |
We use self.path.rstrip('/') a lot. Can we save that somewhere? |
david | |
Please use single quotes here. |
david | |
Same re: docstring formatting. |
david | |
Please use single quotes here. |
david | |
Please use single quotes here. Also, is there a more specific error that can be logged? This should probably at … |
david | |
If the process fails, this should raise an SCMError. |
david | |
Same re: docstring formatting. |
david | |
If the process fails, this should raise an SCMError. |
david | |
Same re: docstring formatting. |
david | |
Please use single quotes here. |
david | |
Please use single quotes here. Also please include the revision in the error output. We should also raise an SCMError |
david | |
Please don't put these all on different lines. |
brennie | |
Does this return an ISO 8601 date or parse an ISO 8601 date? Also, needs an args and return block: … |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
This should be indented like: results.append(Branch( id=data['branch'], ...) |
brennie | |
Needs Args and Returns. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Needs Args and Returns |
brennie | |
blank line between these. |
brennie | |
blank line between these. |
brennie | |
blank line between these. |
brennie | |
blank line between these. |
brennie | |
Args and Returns |
brennie | |
Blank line between these. |
brennie | |
You should pass interpolation args as args to error, e.g. logging.error('foo %s', bar) |
brennie | |
Why is this a string? |
brennie | |
Missing returns |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
See above comment about indentation. |
brennie | |
Args and Returns |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
See above comment about indentation. |
brennie | |
Args and returns. |
brennie | |
BLank line between these. |
brennie | |
Blank line between these. |
brennie | |
The format here isn't quite right, I think. It looks like the contents of data are ints, not strings. This … |
david | |
Should be formatted as such: Returns: list of reviewboard.scmtools.core.Branch: A list of the active branches. |
david | |
Should be formatted as: Args: revision (str): Identifier of changeset. branch (str): An optional branch name to filter by. Returns: … |
david | |
Please add Args/Returns. |
david | |
Description should go on the next line, indented 4 spaces. |
david | |
Description should go on the next line. |
david | |
Should have "list of reviewboard.scmtools.core.Branch" and the description on the next line. |
david | |
Can you put parens around the conditional? That way it's a little more readable about what the parameter value is. |
david | |
Add a period at the end of this sentence. |
david | |
Add a blank line in here. |
david | |
Add a period at the end of this sentence. |
david | |
Add a period at the end of this sentence. |
david | |
Add a period at the end of this sentence. |
david | |
Add a period at the end of this sentence. |
david | |
Col: 12 E111 indentation is not a multiple of four |
reviewbot | |
Col: 12 E111 indentation is not a multiple of four |
reviewbot | |
Col: 31 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 31 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 31 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 31 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 31 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 80 E501 line too long (85 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (85 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (85 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (93 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (85 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (85 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (93 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (85 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (87 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot | |
This should be the last import in the group. from foo import bar goes after import foo. |
brennie | |
Docstrings. |
brennie | |
"date" ? why is this in quotes? |
brennie | |
unicode. |
brennie | |
See other comments about docstrings |
brennie | |
See other comments about docstrings |
brennie | |
unicode. |
brennie | |
can you add the exception to this, e.g. logging.exception('Cannot load details of changeset from hgweb: %s', e) |
brennie | |
See other comment about your docstrings. |
brennie | |
unicode |
brennie | |
Blank line between these. |
brennie | |
unicode |
brennie | |
"Return detailed..." |
brennie | |
Can you improve this, e.g. This method retrieves the patch in JSON format from hgweb or similar. |
brennie | |
unicode |
brennie | |
"Return" instead of "Get" |
brennie | |
This should mention it runs hg and parses the result out, because the method name does not indicate that it … |
brennie | |
This should be a phrase in the imperitive mood. The first word should be a verb. |
brennie | |
list of unicode |
brennie | |
commit |
brennie | |
Can you reformat this last line as: parent=p, base_commit_id=p )) |
brennie | |
unicode |
brennie | |
Blank line between these. |
brennie | |
unicode |
brennie | |
commit |
brennie | |
blank line between these. |
brennie | |
'-r%s:0' % start |
brennie | |
Reformat as: cmd = [ 'log', revset, # ..., ] |
brennie | |
Blank line between these. |
brennie | |
You can replace this with: cmd.extend(('-b', branch)). |
brennie | |
See other comments about docstrings |
brennie | |
unicode. |
brennie | |
Shouldn't this take revision not cmd? |
brennie | |
Incorrect docstring. |
brennie | |
Can we sort these alphabetically? |
david | |
Should be unicode, optional, since there's a default. |
david | |
Should be unicode, optional, since there's a default. |
david | |
Maybe call this date_tuple_to_iso8601? |
david | |
If you move the results assignment to the top, you could have this be return results |
david | |
This whole thing could be a single list comprehension: results = [ Branch( id=data['branch'] commit=data['node'], default=(data['branch'] == 'default')) for data … |
david | |
Can the revision include any special characters in it? Should revision be URL-encoded? |
david | |
unicode, optional |
david | |
unicode, optional |
david | |
This kind of query concatenation is a little error-prone for future changes. Can we instead do something like this? query_parts … |
david | |
query should be urlencoded here, no? |
david | |
Should revision be URL-encoded? |
david | |
Returning an empty list here seems wrong. I think it's more correct to raise an SCMError. |
david | |
Same here. |
david | |
Checking truthiness here isn't really clear. How about if p.wait() != 0:? |
david | |
This can all be a list comprehension. |
david | |
Same here with being explicit in the comparison. |
david | |
unicode, optional |
david | |
unicode, optional |
david | |
This returns a list of Commit, no? |
david | |
Please compare explicitly against 0. |
david | |
This line shouldn't be necessary. We don't actually use contents until after the try/except block in which it's assigned. |
david | |
When would changeset be falsy? Can't we just always append it? |
david | |
I don't think we should be decoding this. The diff should stay as bytes so that it will work correctly … |
david |
-
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! :-)
-
-
-
-
-
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')))
-
-
Can we make this use
logging.exception
, and flesh out the log message a bit to include some mention of mercurial? -
-
-
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?
-
-
-
Can we make this use
logging.exception
, and flesh out the log message a bit to include some mention of mercurial? -
-
-
-
-
-
-
-
-
-
-
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
- Change Summary:
-
Adjust date of changesets to show the correct time (use timezone data)
- Diff:
-
Revision 4 (+184 -2)
-
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. -
-
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. Also, is there a more specific error that can be logged? This should probably at least include the revision.
-
-
-
-
-
-
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
-
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.
-
-
-
-
-
Tool: Pyflakes Processed Files: reviewboard/scmtools/hg.py Tool: PEP8 Style Checker Processed Files: reviewboard/scmtools/hg.py
-
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 toget_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).
- Change Summary:
-
Added tests, fixed issues, added better commit message and replace work-around of hgweb with revset syntax
- Description:
-
~ - HgWebClient:
Support for different branches is not perfect as
hgweb does not support revsets as parameter if
the style 'json' is used.
~ - HgLocalClient:
Fully supported!
~ 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 -Tjson. See: https://selenic.com/repo/hg/help/hgweb
Bugs closed: 4032
- HgWebClient:
- Commit:
-
8330dd2efa926a4701add0f5618c7b8033a45403
-
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
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Description:
-
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 -Tjson. ~ interface via cmdline --template json. See: https://selenic.com/repo/hg/help/hgweb
Bugs closed: 4032
- Commit:
-
4a634d89bf28a2a9d5f37cee34164017220c173fff44478f383d23dfe82c71f4bf45c67ae0717e19
-
-
-
-
-
-
-
-
-
can you add the exception to this, e.g.
logging.exception('Cannot load details of changeset from hgweb: %s', e)
-
-
-
-
-
-
-
-
-
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.
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Change Summary:
-
fix issues
- Commit:
-
ff44478f383d23dfe82c71f4bf45c67ae0717e1915ca3f006999c114bacddf618c31225871102da4
-
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
-
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
-
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
- Change Summary:
-
fix doc
- Commit:
-
f8a55b7fdc6d6234d5deead80ee288232b1dcf80515a2417bf651641b813fbd5d9d4d9fb32ebeaba
-
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
-
-
-
-
-
-
-
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' ]
-
-
-
-
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)
-
-
-
-
-
-
-
-
-
-
-
-
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
-
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