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#newcode162
tools/release/search_related_commits.py:162:
(_git_execute(options.git_dir,
On 2015/04/27 13:09:43, Michael Achenbach wrote:
On 2015/04/24 12:45:12, Michael Achenbach wrote:
> nit: too many parenthesis
What about this? Maybe also move the lambda into a local variable for
better
readability.
I still have the feeling this comment was overlooked.
https://codereview.chromium.org/1098123002/diff/120001/tools/release/search_related_commits.py
File tools/release/search_related_commits.py (right):
https://codereview.chromium.org/1098123002/diff/120001/tools/release/search_related_commits.py#newcode22
tools/release/search_related_commits.py:22: #Adding start hash too
nit: Space after # - end comments with periods.
https://codereview.chromium.org/1098123002/diff/120001/tools/release/search_related_commits.py#newcode78
tools/release/search_related_commits.py:78: git_working_dir,(
nit: space after comma
https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_search_related_commits.py
File tools/release/test_search_related_commits.py (right):
https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_search_related_commits.py#newcode6
tools/release/test_search_related_commits.py:6: import unittest
nit sort by module name
https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_search_related_commits.py#newcode22
tools/release/test_search_related_commits.py:22: def _execute_git(self,
commands):
Maybe use *args for commands (e.g. *commands), then you don't need lists
below?
https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_search_related_commits.py#newcode35
tools/release/test_search_related_commits.py:35: f = open(self.base_dir
+ "/" + file_name, 'w')
Use "with" statement
https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_search_related_commits.py#newcode39
tools/release/test_search_related_commits.py:39:
self._execute_git(["add", self.base_dir + "/" + file_name])
Do you need files? There is a flag that allows empty commits
(--allow-empty?). Do files give any benefit for these tests?
https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_search_related_commits.py#newcode46
tools/release/test_search_related_commits.py:46: check_call(["git",
"init", self.base_dir])
I like this testing approach! Will look into the details tomorrow.
https://codereview.chromium.org/1098123002/diff/120001/tools/release/test_search_related_commits.py#newcode94
tools/release/test_search_related_commits.py:94: f = open(self.base_dir
+ "/first", 'w')
use "with" statement and/or extract some helper methods for similar
stuff. This seems recurring.
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.
