Some comments. Will make another review round after you looked through
those.
Many of the formatting comments relate to the whole file (I didn't comment
on
all instances).
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):
nit: 4 space indentation for params.
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):
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.
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:
nit:
if related_commits:
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
already_treated_commits.extend(related_commits)
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode40
tools/release/search_related_commits.py:40: if deadline != "":
Better no default for deadline (i.e. default is None) and then:
if deadline:
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode41
tools/release/search_related_commits.py:41: afterDeadline = False
unused variable?
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)
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))
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,
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,
...
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode161
tools/release/search_related_commits.py:161: high_level_commits =
sorted(all_related_commits.keys(),key = lambda x:(
nit:
,space
:space
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode162
tools/release/search_related_commits.py:162:
(_git_execute(options.git_dir,
nit: too many parenthesis
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode168
tools/release/search_related_commits.py:168: print "+" + (_git_execute(
Difficult to read the way the parenthesis are set and the way it is
indented. Maybe extract method?
https://codereview.chromium.org/1098123002/diff/80001/tools/release/search_related_commits.py#newcode183
tools/release/search_related_commits.py:183: print "| " + (_git_execute(
Same here.
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.
