https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py
File tools/release/search_related_commits.py (right):
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode14
tools/release/search_related_commits.py:14: git_working_dir, of, until,
deadline, verbose=False):
On 2015/04/24 12:45:12, Michael Achenbach wrote:
nit: 4 space indentation for params.
Done.
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode14
tools/release/search_related_commits.py:14: git_working_dir, of, until,
deadline, verbose=False):
On 2015/04/24 12:45:12, Michael Achenbach wrote:
Suggestion: Better name for "of"? It know it sounds fluent as
search_all_related_commits "of", but the single variable "of" in the
code flow
is confusing. I'd like something like "revision" better.
Renamed it inside the function.
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode30
tools/release/search_related_commits.py:30: if len(related_commits) > 0:
On 2015/04/24 12:45:12, Michael Achenbach wrote:
nit:
if related_commits:
I like it that way as it is more explicit and better readable.
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode32
tools/release/search_related_commits.py:32: already_treated_commits =
already_treated_commits + related_commits
On 2015/04/24 12:45:12, Michael Achenbach wrote:
already_treated_commits.extend(related_commits)
Done.
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode40
tools/release/search_related_commits.py:40: if deadline != "":
On 2015/04/24 12:45:12, Michael Achenbach wrote:
Better no default for deadline (i.e. default is None) and then:
if deadline:
Done.
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode41
tools/release/search_related_commits.py:41: afterDeadline = False
On 2015/04/24 12:45:12, Michael Achenbach wrote:
unused variable?
Done.
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode96
tools/release/search_related_commits.py:96: hits =
(_convert_to_array(found_by_hash)
On 2015/04/24 12:45:12, Michael Achenbach wrote:
nit: Indentation: Either use 4 space indentation or same level,
operators on
last line ending:
hits = (
_convert_to_array(found_by_hash) +
_convert_to_array(found_by_commit_pos) +
_convert_to_array(found_by_title)
)
or
hits = (_convert_to_array(found_by_hash) +
_convert_to_array(found_by_commit_pos) +
_convert_to_array(found_by_title))
Done.
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode155
tools/release/search_related_commits.py:155: all_related_commits =
(search_all_related_commits(options.git_dir,
On 2015/04/24 12:45:12, Michael Achenbach wrote:
nit: Doesn't need the outer parenthesis + params all on one level:
all_related_commits = search_all_related_commits(
options.git_dir,
...
Or indent like this:
all_related_commits = (
search_all_related_commits(
options.git_dir,
...
Done.
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode168
tools/release/search_related_commits.py:168: print "+" + (_git_execute(
On 2015/04/24 12:45:12, Michael Achenbach wrote:
Difficult to read the way the parenthesis are set and the way it is
indented.
Maybe extract method?
Done.
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode183
tools/release/search_related_commits.py:183: print "| " + (_git_execute(
On 2015/04/24 12:45:12, Michael Achenbach wrote:
Same here.
Done.
https://codereview.chromium.org/1098123002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
