First round of comments. I haven't looked at the tests in much detail yet;
I'll
do that on the next patch set.
https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_includes.py
File tools/push-to-trunk/common_includes.py (right):
https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_includes.py#newcode102
tools/push-to-trunk/common_includes.py:102: out = filter(lambda x:
re.search(r"^[ \t]*BUG[ \t]*=", x), out)
I don't think we need any of these three "filter" lines any more.
AddIssues() below should handle all of that anyway.
https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_includes.py#newcode109
tools/push-to-trunk/common_includes.py:109: def AddSafe(bugs, bug):
I think you don't need this function at all, if below in the regexes you
match against (\d+) instead of (.*).
https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_includes.py#newcode116
tools/push-to-trunk/common_includes.py:116: ref = re.match(r"^[ \t]*BUG[
\t]*=[ \t]*(.*?)[ \t]*$", text)
I think the regex can be simplified to:
r"^[ \t]*BUG=(.*?)$"
Reasons:
- whitespace around '=' doesn't happen in practice
- whitespace at the end would be strip()'ed away below anyway.
- the simpler a regex, the better :-)
https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_includes.py#newcode121
tools/push-to-trunk/common_includes.py:121: match = re.match(r"^v8[
\t]*:[ \t]*(.*)$", bug)
similarly, we don't need to support whitespace around ':'
https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_includes.py#newcode141
tools/push-to-trunk/common_includes.py:141: FormatIssues("", v8bugs)
V8 bugs first, please :-)
https://codereview.chromium.org/65933003/
--
--
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/groups/opt_out.
