Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
421223003:
[Findit] Plain objects to represent the returned result from running the algorithm, (Closed)
Description[Findit] Plain objects to represent the returned result from running the algorithm,
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289949
Patch Set 1 #
Total comments: 64
Patch Set 2 : addressed codereview #
Total comments: 5
Patch Set 3 : addressed code review / added git support. #
Total comments: 4
Patch Set 4 : Addressed codereview. #Patch Set 5 : fixed bug on blame prettify #Patch Set 6 : Refactored matchset/blame and added object result to represent both of them. #
Total comments: 8
Patch Set 7 : added JSON format #
Total comments: 4
Patch Set 8 : addressed codereview. #
Total comments: 44
Patch Set 9 : addressed code review #
Messages
Total messages: 21 (0 generated)
https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py File tools/findit/blame.py (right): https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode4 tools/findit/blame.py:4: from threading import Lock, Thread Please add an empty line before this one. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode6 tools/findit/blame.py:6: import findit_for_crash_utils as findit_utils Seems not up-to-day. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode8 tools/findit/blame.py:8: import svnparser Hmm, why we use the parsers here? I remember blame.Blame was imported by the parsers instead. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode22 tools/findit/blame.py:22: line: line that caused a crash line_number? https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode26 tools/findit/blame.py:26: url: url of this revision url to the file change or the whole CL change? https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode33 tools/findit/blame.py:33: # Set all the variables from the arguments Style: should end with a dot. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode84 tools/findit/blame.py:84: Only either first 10 or the length of stack, whichever is shorter, How about making it configurable? https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode94 tools/findit/blame.py:94: a list of blame object indent? https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcod... tools/findit/blame.py:102: This empty line could be removed. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcod... tools/findit/blame.py:114: # get it. Not for Git because we cannot calculate the distance style: no ending dot. It is a common issue in this patch. Please check all comments by doing a grep. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcod... tools/findit/blame.py:131: blame_thread.join() It seems nothing is returned. If this is intended, the documentation of this function should be updated. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcod... tools/findit/blame.py:133: def __GenerateBlameList(self, repository_parser, stacktrace_line, This function generate a single Blame only. The name is kinda misleading. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcod... tools/findit/blame.py:166: blame_list.sort(key=lambda blame: (blame.diff, blame.stack)) blame.diff? https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcod... tools/findit/blame.py:169: This empty line could be removed. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py File tools/findit/matchset.py (right): https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:4: import re Add a new line before this one. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:6: import findit_for_crash_utils as findit_utils seems not up-to-day. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:10: pl = Lock() Please use full name. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:22: def GenerateMatchEntry( I think this function is closely-related to the algorithm. I suggest that we move it out to another file. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:27: A match is a CL that is suspected to have caused the crash. A match object It might be better to move the Match class up to the top, and add this explanation in that class instead. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:36: as returned from ParseRevisions ParseRevisions is unclear here. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:49: if cl in self.cls_to_avoid: avoid -> ignore? https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:58: match.ParseMessage(message, self.codereview_api_url) This function call will be time-consuming as it issues http request. It is better to not holding the lock here. It is fine for the moment. But please add a TODO here to resolve this problem. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:66: self.cls_to_avoid.add(match.revert_of) What if the cl revert_of was already analyzed and in the list |self.matches|? https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:76: self.matches_lock.release() If code above raise an exception, the lock won't be released. Maybe use a with clause instead. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:84: (line_intersection, stack_frame_index_intersection) = findit_utils.Intersection( style: > 80 chars. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:134: file: list of files that the CL changed files? https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:135: file_url: list of URLs for the file file_urls? https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:138: stack_frame_index: for files that caused crash, list of where in the The name doesn't indicate it is a list. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:140: rank: the highest priority among the files the CL changes What's the concept of priority here? Would you mind adding an explanation? https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:144: review_address: a codereview address that reviews this CL review_url? https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:187: review_number = self.review_address.split('/')[-1] Usually, it is called issue number.
https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py File tools/findit/blame.py (right): https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode4 tools/findit/blame.py:4: from threading import Lock, Thread On 2014/08/04 20:44:56, Shuotao wrote: > Please add an empty line before this one. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode6 tools/findit/blame.py:6: import findit_for_crash_utils as findit_utils On 2014/08/04 20:44:56, Shuotao wrote: > Seems not up-to-day. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode8 tools/findit/blame.py:8: import svnparser On 2014/08/04 20:44:56, Shuotao wrote: > Hmm, why we use the parsers here? > > I remember blame.Blame was imported by the parsers instead. I have changed the code that parsers do not use them anymore. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode22 tools/findit/blame.py:22: line: line that caused a crash On 2014/08/04 20:44:56, Shuotao wrote: > line_number? Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode26 tools/findit/blame.py:26: url: url of this revision On 2014/08/04 20:44:56, Shuotao wrote: > url to the file change or the whole CL change? This revision, the whole CL change. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode33 tools/findit/blame.py:33: # Set all the variables from the arguments On 2014/08/04 20:44:56, Shuotao wrote: > Style: should end with a dot. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode84 tools/findit/blame.py:84: Only either first 10 or the length of stack, whichever is shorter, On 2014/08/04 20:44:56, Shuotao wrote: > How about making it configurable? Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode94 tools/findit/blame.py:94: a list of blame object On 2014/08/04 20:44:56, Shuotao wrote: > indent? Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcod... tools/findit/blame.py:102: On 2014/08/04 20:44:57, Shuotao wrote: > This empty line could be removed. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcod... tools/findit/blame.py:114: # get it. Not for Git because we cannot calculate the distance On 2014/08/04 20:44:56, Shuotao wrote: > style: no ending dot. > > It is a common issue in this patch. > Please check all comments by doing a grep. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcod... tools/findit/blame.py:131: blame_thread.join() On 2014/08/04 20:44:57, Shuotao wrote: > It seems nothing is returned. > If this is intended, the documentation of this function should be updated. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcod... tools/findit/blame.py:133: def __GenerateBlameList(self, repository_parser, stacktrace_line, On 2014/08/04 20:44:56, Shuotao wrote: > This function generate a single Blame only. The name is kinda misleading. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcod... tools/findit/blame.py:166: blame_list.sort(key=lambda blame: (blame.diff, blame.stack)) On 2014/08/04 20:44:56, Shuotao wrote: > blame.diff? changed to distance https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcod... tools/findit/blame.py:169: On 2014/08/04 20:44:57, Shuotao wrote: > This empty line could be removed. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py File tools/findit/matchset.py (right): https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:4: import re On 2014/08/04 20:44:57, Shuotao wrote: > Add a new line before this one. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:6: import findit_for_crash_utils as findit_utils On 2014/08/04 20:44:57, Shuotao wrote: > seems not up-to-day. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:10: pl = Lock() On 2014/08/04 20:44:57, Shuotao wrote: > Please use full name. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:22: def GenerateMatchEntry( On 2014/08/04 20:44:58, Shuotao wrote: > I think this function is closely-related to the algorithm. > > I suggest that we move it out to another file. We can talk about this. I feel like this function can stay here. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:27: A match is a CL that is suspected to have caused the crash. A match object On 2014/08/04 20:44:57, Shuotao wrote: > It might be better to move the Match class up to the top, and add this > explanation in that class instead. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:36: as returned from ParseRevisions On 2014/08/04 20:44:57, Shuotao wrote: > ParseRevisions is unclear here. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:49: if cl in self.cls_to_avoid: On 2014/08/04 20:44:57, Shuotao wrote: > avoid -> ignore? Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:58: match.ParseMessage(message, self.codereview_api_url) On 2014/08/04 20:44:58, Shuotao wrote: > This function call will be time-consuming as it issues http request. > It is better to not holding the lock here. > > It is fine for the moment. > But please add a TODO here to resolve this problem. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:66: self.cls_to_avoid.add(match.revert_of) On 2014/08/04 20:44:58, Shuotao wrote: > What if the cl revert_of was already analyzed and in the list |self.matches|? I remove all reverts at the end again. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:76: self.matches_lock.release() On 2014/08/04 20:44:57, Shuotao wrote: > If code above raise an exception, the lock won't be released. > > Maybe use a with clause instead. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:84: (line_intersection, stack_frame_index_intersection) = findit_utils.Intersection( On 2014/08/04 20:44:58, Shuotao wrote: > style: > 80 chars. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:134: file: list of files that the CL changed On 2014/08/04 20:44:58, Shuotao wrote: > files? Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:135: file_url: list of URLs for the file On 2014/08/04 20:44:57, Shuotao wrote: > file_urls? Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:138: stack_frame_index: for files that caused crash, list of where in the On 2014/08/04 20:44:57, Shuotao wrote: > The name doesn't indicate it is a list. Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:140: rank: the highest priority among the files the CL changes On 2014/08/04 20:44:57, Shuotao wrote: > What's the concept of priority here? > Would you mind adding an explanation? Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:144: review_address: a codereview address that reviews this CL On 2014/08/04 20:44:57, Shuotao wrote: > review_url? Done. https://codereview.chromium.org/421223003/diff/1/tools/findit/matchset.py#new... tools/findit/matchset.py:187: review_number = self.review_address.split('/')[-1] On 2014/08/04 20:44:57, Shuotao wrote: > Usually, it is called issue number. Done.
https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py File tools/findit/blame.py (right): https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode26 tools/findit/blame.py:26: url: url of this revision On 2014/08/06 21:33:04, jeun wrote: > On 2014/08/04 20:44:56, Shuotao wrote: > > url to the file change or the whole CL change? > This revision, the whole CL change. In that case, please update the documentation here and make it a little clear. https://codereview.chromium.org/421223003/diff/20001/tools/findit/blame.py File tools/findit/blame.py (right): https://codereview.chromium.org/421223003/diff/20001/tools/findit/blame.py#ne... tools/findit/blame.py:85: Only either first 10 or the length of stack, whichever is shorter, 10 here is invalid. Same for the in-line comment below. Whenever you make code change, please ensure comments and documentation are updated. This seems to become a common issues in most of your code. https://codereview.chromium.org/421223003/diff/20001/tools/findit/blame.py#ne... tools/findit/blame.py:102: for stack_frame in stackframes: Naming is not that consistent. |stackframes| -> |stack_frames| https://codereview.chromium.org/421223003/diff/20001/tools/findit/matchset.py File tools/findit/matchset.py (right): https://codereview.chromium.org/421223003/diff/20001/tools/findit/matchset.py... tools/findit/matchset.py:146: if match.isrevert: |isrevert| -> |is_reverted|?
Hi Shoutao, I have addressed the code review. Would you mind taking another look? Thanks! https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py File tools/findit/blame.py (right): https://codereview.chromium.org/421223003/diff/1/tools/findit/blame.py#newcode26 tools/findit/blame.py:26: url: url of this revision On 2014/08/09 19:51:02, Shuotao wrote: > On 2014/08/06 21:33:04, jeun wrote: > > On 2014/08/04 20:44:56, Shuotao wrote: > > > url to the file change or the whole CL change? > > This revision, the whole CL change. > > In that case, please update the documentation here and make it a little clear. Done. https://codereview.chromium.org/421223003/diff/20001/tools/findit/blame.py File tools/findit/blame.py (right): https://codereview.chromium.org/421223003/diff/20001/tools/findit/blame.py#ne... tools/findit/blame.py:85: Only either first 10 or the length of stack, whichever is shorter, On 2014/08/09 19:51:02, Shuotao wrote: > 10 here is invalid. Same for the in-line comment below. > > Whenever you make code change, please ensure comments and documentation are > updated. > This seems to become a common issues in most of your code. Done. https://codereview.chromium.org/421223003/diff/20001/tools/findit/blame.py#ne... tools/findit/blame.py:102: for stack_frame in stackframes: On 2014/08/09 19:51:02, Shuotao wrote: > Naming is not that consistent. > |stackframes| -> |stack_frames| Done.
Hi Jason, The CL looks good to me. I just posted two small comments. Feel free to ask CF folks for code review. Thanks, Shuotao Gao https://codereview.chromium.org/421223003/diff/40001/tools/findit/blame.py File tools/findit/blame.py (right): https://codereview.chromium.org/421223003/diff/40001/tools/findit/blame.py#ne... tools/findit/blame.py:81: url_map, n=10): Maybe rename "n" to "top_n_frames"? https://codereview.chromium.org/421223003/diff/40001/tools/findit/matchset.py File tools/findit/matchset.py (right): https://codereview.chromium.org/421223003/diff/40001/tools/findit/matchset.py... tools/findit/matchset.py:1: # Copyright (c) 2014 The Chromium Authors. All rights reserved. Rename file to "match_set.py"?
Hi Shuotao, I have addressed the code review. Marty and Abhishek, can you take a look at the code please? Thank you in advance! https://codereview.chromium.org/421223003/diff/40001/tools/findit/blame.py File tools/findit/blame.py (right): https://codereview.chromium.org/421223003/diff/40001/tools/findit/blame.py#ne... tools/findit/blame.py:81: url_map, n=10): On 2014/08/12 19:12:18, Shuotao wrote: > Maybe rename "n" to "top_n_frames"? Done. https://codereview.chromium.org/421223003/diff/40001/tools/findit/matchset.py File tools/findit/matchset.py (right): https://codereview.chromium.org/421223003/diff/40001/tools/findit/matchset.py... tools/findit/matchset.py:1: # Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/08/12 19:12:18, Shuotao wrote: > Rename file to "match_set.py"? Done.
Hi Shuotao, I have refactored the code and added the result object. Would you mind taking a look? Thanks!
After addressing my comments, this CL could be committed today. https://codereview.chromium.org/421223003/diff/100001/tools/findit/blame.py File tools/findit/blame.py (right): https://codereview.chromium.org/421223003/diff/100001/tools/findit/blame.py#n... tools/findit/blame.py:106: repository_parser = parsers[1 if is_git else 0] Could we decide which parser should be used in the caller of this function, and then just pass the correct parser into this function? https://codereview.chromium.org/421223003/diff/100001/tools/findit/match_set.py File tools/findit/match_set.py (right): https://codereview.chromium.org/421223003/diff/100001/tools/findit/match_set.... tools/findit/match_set.py:28: stackframe they occur. indent? https://codereview.chromium.org/421223003/diff/100001/tools/findit/match_set.... tools/findit/match_set.py:32: url: URL of the CL. |url| is not that clear. Please find a better name for it. https://codereview.chromium.org/421223003/diff/100001/tools/findit/match_set.... tools/findit/match_set.py:33: review_url: The codereview address that reviews this CL. address -> url?
Hi Shuotao, I have addressed the codereview and added JSON format support. thanks! https://codereview.chromium.org/421223003/diff/100001/tools/findit/blame.py File tools/findit/blame.py (right): https://codereview.chromium.org/421223003/diff/100001/tools/findit/blame.py#n... tools/findit/blame.py:106: repository_parser = parsers[1 if is_git else 0] On 2014/08/14 16:31:19, Shuotao wrote: > Could we decide which parser should be used in the caller of this function, and > then just pass the correct parser into this function? FindBlame is a function that finds blame for whole callstack, and callstack is composed of frames that are most likely from different components. So this function should have both (all) parsers. Another way might be to just import parsers in this file and create new parser here, but i did not want to do that. https://codereview.chromium.org/421223003/diff/100001/tools/findit/match_set.py File tools/findit/match_set.py (right): https://codereview.chromium.org/421223003/diff/100001/tools/findit/match_set.... tools/findit/match_set.py:28: stackframe they occur. On 2014/08/14 16:31:19, Shuotao wrote: > indent? Done. https://codereview.chromium.org/421223003/diff/100001/tools/findit/match_set.... tools/findit/match_set.py:32: url: URL of the CL. On 2014/08/14 16:31:19, Shuotao wrote: > |url| is not that clear. Please find a better name for it. Done. https://codereview.chromium.org/421223003/diff/100001/tools/findit/match_set.... tools/findit/match_set.py:33: review_url: The codereview address that reviews this CL. On 2014/08/14 16:31:19, Shuotao wrote: > address -> url? Done.
One more nit. Please also update the description and subject of this CL. Use the "Edit Issue" on the left-top. https://codereview.chromium.org/421223003/diff/120001/tools/findit/result.py File tools/findit/result.py (right): https://codereview.chromium.org/421223003/diff/120001/tools/findit/result.py#... tools/findit/result.py:25: 'suspected cl revision url': self.suspected_cl_revision_url, Could we use a shorter name like just 'revision_url'? https://codereview.chromium.org/421223003/diff/120001/tools/findit/result.py#... tools/findit/result.py:35: This empty line could be removed.
Hi Shuotao, I have updated the files as suggested. Thanks! https://codereview.chromium.org/421223003/diff/120001/tools/findit/result.py File tools/findit/result.py (right): https://codereview.chromium.org/421223003/diff/120001/tools/findit/result.py#... tools/findit/result.py:25: 'suspected cl revision url': self.suspected_cl_revision_url, On 2014/08/14 17:47:15, Shuotao wrote: > Could we use a shorter name like just 'revision_url'? Done. https://codereview.chromium.org/421223003/diff/120001/tools/findit/result.py#... tools/findit/result.py:35: On 2014/08/14 17:47:15, Shuotao wrote: > This empty line could be removed. Done.
lgtm
Hi Jason, You could check the "commit" box to make this CL into Commit Queue. Thanks, Shuotao Gao
https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py File tools/findit/blame.py (right): https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:17: content: The content of the line to find the blame for. s/content/line_content https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:48: # Calculate the distance, where it measures how far the last revision is Can you stop by and help me understand what do you mean and how are using distance all over the place. https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:96: # It is not possible to get blame information so ignore this line. s/It/it s/so/, so https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:106: repository_parser = parsers[1 if is_git else 0] parsers should be a dict. parsers['git'] and parsers['svn'], don't use 0 and 1. https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:128: def __GenerateBlameEntry(self, repository_parser, stack_frame, Why does the name start with __ https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:136: line = stack_frame.crashed_line_number s/crashed_line_number/crash_line_number s/line/crash_line_number . better to use same naming convention. https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:143: if not parsed_blame_info: can you check list length so that we don't error on next line. https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:171: if (blame.distance > blame.range_start / 4) and ( This rule makes no sense, why are you adding this or why is this needed. How has this helped in results. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.py File tools/findit/match_set.py (right): https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:19: line_of_crash: The list of lines that caused crash for this CL. s/line_of_crash/crash_lines https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:20: function: The list of functions that caused the crash. s/function/function_list https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:71: if line.startswith('Review URL: '): Put 'Review URL: ' in a global var and use it in both places. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:117: def RemoveReverts(self): s/RemoveReverts/RemoveRevertedCLs
some more comments, make sure to fix the old ones too (in last comment.) https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py File tools/findit/blame.py (right): https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:18: component_name: The name of the component this line is in. s/this line is in/for this line. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.py File tools/findit/match_set.py (right): https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:7: from threading import Lock blank line before this line. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:21: min_distance: The minimum difference between the lines that CL changed and s/difference/distance. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:23: files: The list of files that the CL changed. s/files/changed_files https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:24: file_urls: The list of URLs for the file. s/file_urls/changed_files_urls https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:29: rank: The highest priority among the files the CL changes. Explain meaning of priorities. Priority = 0 means what, 1 means what, etc. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:30: priorities: For each files, whether it changes the crashed line s/For each files/For each file https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:40: self.is_reverted = False put these two lines in descriptions. is_reverted and revert_of is not explained above. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:54: self.reviewers = ['N/A'] Why N/A and not empty list. also same for review url, why not '', instead of N/A https://codereview.chromium.org/421223003/diff/140001/tools/findit/result.py File tools/findit/result.py (right): https://codereview.chromium.org/421223003/diff/140001/tools/findit/result.py#... tools/findit/result.py:21: def ToDictionary(self): Why is this needed ? we can just look this up in result object.
Hi, I have addressed the code review. Thanks! https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py File tools/findit/blame.py (right): https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:17: content: The content of the line to find the blame for. On 2014/08/14 20:22:20, aarya wrote: > s/content/line_content Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:18: component_name: The name of the component this line is in. On 2014/08/14 21:14:39, aarya wrote: > s/this line is in/for this line. Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:48: # Calculate the distance, where it measures how far the last revision is On 2014/08/14 20:22:20, aarya wrote: > Can you stop by and help me understand what do you mean and how are using > distance all over the place. Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:96: # It is not possible to get blame information so ignore this line. On 2014/08/14 20:22:20, aarya wrote: > s/It/it > s/so/, so Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:106: repository_parser = parsers[1 if is_git else 0] On 2014/08/14 20:22:20, aarya wrote: > parsers should be a dict. parsers['git'] and parsers['svn'], don't use 0 and 1. Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:128: def __GenerateBlameEntry(self, repository_parser, stack_frame, On 2014/08/14 20:22:21, aarya wrote: > Why does the name start with __ It is because the function is used only in this class. https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:136: line = stack_frame.crashed_line_number On 2014/08/14 20:22:20, aarya wrote: > s/crashed_line_number/crash_line_number > s/line/crash_line_number . better to use same naming convention. Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:143: if not parsed_blame_info: On 2014/08/14 20:22:21, aarya wrote: > can you check list length so that we don't error on next line. Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/blame.py#n... tools/findit/blame.py:171: if (blame.distance > blame.range_start / 4) and ( On 2014/08/14 20:22:21, aarya wrote: > This rule makes no sense, why are you adding this or why is this needed. How has > this helped in results. Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.py File tools/findit/match_set.py (right): https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:7: from threading import Lock On 2014/08/14 21:14:39, aarya wrote: > blank line before this line. Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:19: line_of_crash: The list of lines that caused crash for this CL. On 2014/08/14 20:22:21, aarya wrote: > s/line_of_crash/crash_lines Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:20: function: The list of functions that caused the crash. On 2014/08/14 20:22:21, aarya wrote: > s/function/function_list Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:21: min_distance: The minimum difference between the lines that CL changed and On 2014/08/14 21:14:40, aarya wrote: > s/difference/distance. Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:23: files: The list of files that the CL changed. On 2014/08/14 21:14:40, aarya wrote: > s/files/changed_files Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:24: file_urls: The list of URLs for the file. On 2014/08/14 21:14:40, aarya wrote: > s/file_urls/changed_files_urls Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:29: rank: The highest priority among the files the CL changes. On 2014/08/14 21:14:40, aarya wrote: > Explain meaning of priorities. Priority = 0 means what, 1 means what, etc. Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:30: priorities: For each files, whether it changes the crashed line On 2014/08/14 21:14:39, aarya wrote: > s/For each files/For each file Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:40: self.is_reverted = False On 2014/08/14 21:14:40, aarya wrote: > put these two lines in descriptions. is_reverted and revert_of is not explained > above. Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:54: self.reviewers = ['N/A'] On 2014/08/14 21:14:40, aarya wrote: > Why N/A and not empty list. also same for review url, why not '', instead of N/A Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:71: if line.startswith('Review URL: '): On 2014/08/14 20:22:21, aarya wrote: > Put 'Review URL: ' in a global var and use it in both places. Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/match_set.... tools/findit/match_set.py:117: def RemoveReverts(self): On 2014/08/14 20:22:21, aarya wrote: > s/RemoveReverts/RemoveRevertedCLs Done. https://codereview.chromium.org/421223003/diff/140001/tools/findit/result.py File tools/findit/result.py (right): https://codereview.chromium.org/421223003/diff/140001/tools/findit/result.py#... tools/findit/result.py:21: def ToDictionary(self): On 2014/08/14 21:14:40, aarya wrote: > Why is this needed ? we can just look this up in result object. Done.
lgtm
The CQ bit was checked by jeun@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeun@chromium.org/421223003/200001
Message was sent while issue was closed.
Committed patchset #9 (200001) as 289949 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
