Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2435863003:
[Findit] Add local git parsers. (Closed)
|
Created:
4 years, 2 months ago by Sharu Jiang Modified:
4 years, 1 month ago CC:
chromium-reviews, infra-reviews+infra_chromium.org Target Ref:
refs/heads/master Project:
infra Visibility:
Public. |
Description[Findit] Add local git parsers.
BUG=605369
Committed: https://chromium.googlesource.com/infra/infra/+/b8b4590396043079125412a7e3715c7867eef05f
Patch Set 1 #Patch Set 2 : Fix nits. #
Total comments: 16
Patch Set 3 : Fix nits. #
Total comments: 6
Patch Set 4 : Address comments. #Patch Set 5 : Fix nits. #
Total comments: 16
Patch Set 6 : Address comments. #Patch Set 7 : Add back TimeZoneInfo. #Patch Set 8 : Fix nits. #Patch Set 9 : Rebase #
Total comments: 13
Patch Set 10 : Split the refatoring part to another cl. #Patch Set 11 : Add comments. #Patch Set 12 : Add __init__ to setup tests for local scripts. #Patch Set 13 : Fix nits. #Patch Set 14 : Fix nits. #
Total comments: 28
Patch Set 15 : Address comments. #Patch Set 16 : Fix nits. #Patch Set 17 : Rebase. #Messages
Total messages: 67 (38 generated)
PTAL :)
Description was changed from ========== [Findit] Add local git parsers. BUG=605369 ========== to ========== [Findit] Add local git parsers. BUG=605369 ==========
katesonia@chromium.org changed reviewers: + chanli@chromium.org, lijeffrey@chromium.org, stgao@chromium.org, wrengr@chromium.org
https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... File appengine/findit/common/local_git_parsers.py (right): https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/local_git_parsers.py:60: offset = int(offset_str[-4:-2]) * 60 + int(offset_str[-2:]) is it possible to move offset manipulation to a separate function? this looks a bit cryptic https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/local_git_parsers.py:117: # Sample: ec3ed6a5ebf6f2c406d7bcf94b6bc34fcaeb976e 2 1 7 pylint will complain about these. how about "ec3ed6... 2 1 7". with the . outside so it's clear what's in the line being read? same with the others. https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/local_git_parsers.py:161: """Gets Change type based on the initial carocter.""" character? https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/local_git_parsers.py:163: return INITIAL_TO_CHANGE_TYPE.get(initial) why not inline initial[0]? https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/local_git_parsers.py:168: if change_type == 'modify': use .lower() before comparing strings https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/local_git_parsers.py:222: change_log.message = change_log.message[:-1] nit: is it safer to first check the last character before chopping it off? https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... File appengine/findit/common/repo_util.py (right): https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/repo_util.py:37: lines = message.strip().split('\n')[-5:] move this to a constant
https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... File appengine/findit/common/local_git_parsers.py (right): https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/local_git_parsers.py:60: offset = int(offset_str[-4:-2]) * 60 + int(offset_str[-2:]) On 2016/10/20 23:31:31, lijeffrey wrote: > is it possible to move offset manipulation to a separate function? this looks a > bit cryptic Done. https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/local_git_parsers.py:117: # Sample: ec3ed6a5ebf6f2c406d7bcf94b6bc34fcaeb976e 2 1 7 On 2016/10/20 23:31:31, lijeffrey wrote: > pylint will complain about these. how about "ec3ed6... 2 1 7". with the . > outside so it's clear what's in the line being read? same with the others. Done. https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/local_git_parsers.py:161: """Gets Change type based on the initial carocter.""" On 2016/10/20 23:31:31, lijeffrey wrote: > character? Oops. https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/local_git_parsers.py:163: return INITIAL_TO_CHANGE_TYPE.get(initial) On 2016/10/20 23:31:32, lijeffrey wrote: > why not inline initial[0]? Done. https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/local_git_parsers.py:168: if change_type == 'modify': On 2016/10/20 23:31:31, lijeffrey wrote: > use .lower() before comparing strings Done. https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/local_git_parsers.py:222: change_log.message = change_log.message[:-1] On 2016/10/20 23:31:32, lijeffrey wrote: > nit: is it safer to first check the last character before chopping it off? Since this '\n' is manually added at last, so no need for checking. https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... File appengine/findit/common/repo_util.py (right): https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/repo_util.py:37: lines = message.strip().split('\n')[-5:] On 2016/10/20 23:31:32, lijeffrey wrote: > move this to a constant Cannot think of a good name except a extremely long one: COMMIT_POSITION_CODE_REVIEW_URL_PARSING_START_LINE
https://codereview.chromium.org/2435863003/diff/40001/appengine/findit/common... File appengine/findit/common/blame.py (right): https://codereview.chromium.org/2435863003/diff/40001/appengine/findit/common... appengine/findit/common/blame.py:31: super(Blame, self).__init__(regions or []) Can we just use the AddRegion below or remove AddRegion if we want to pass the regions in the constructor? This doesn't look that clean to me. https://codereview.chromium.org/2435863003/diff/40001/appengine/findit/common... File appengine/findit/common/change_log.py (right): https://codereview.chromium.org/2435863003/diff/40001/appengine/findit/common... appengine/findit/common/change_log.py:30: revision=None, commit_position=None, message=None, Why all these fields become optional? An't they all required? https://codereview.chromium.org/2435863003/diff/40001/appengine/findit/common... File appengine/findit/common/test/time_util_test.py (right): https://codereview.chromium.org/2435863003/diff/40001/appengine/findit/common... appengine/findit/common/test/time_util_test.py:60: self.assertEqual(tz.utcoffset, timedelta(minutes=480)) +chanli: is this correct? I remembered you worked on some similar code?
https://codereview.chromium.org/2435863003/diff/40001/appengine/findit/common... File appengine/findit/common/blame.py (right): https://codereview.chromium.org/2435863003/diff/40001/appengine/findit/common... appengine/findit/common/blame.py:31: super(Blame, self).__init__(regions or []) On 2016/10/22 00:39:16, stgao wrote: > Can we just use the AddRegion below or remove AddRegion if we want to pass the > regions in the constructor? > > This doesn't look that clean to me. This is removed in the newest patch. https://codereview.chromium.org/2435863003/diff/40001/appengine/findit/common... File appengine/findit/common/change_log.py (right): https://codereview.chromium.org/2435863003/diff/40001/appengine/findit/common... appengine/findit/common/change_log.py:30: revision=None, commit_position=None, message=None, On 2016/10/22 00:39:16, stgao wrote: > Why all these fields become optional? An't they all required? Right, should use FromDict instead. Done. https://codereview.chromium.org/2435863003/diff/40001/appengine/findit/common... File appengine/findit/common/test/time_util_test.py (right): https://codereview.chromium.org/2435863003/diff/40001/appengine/findit/common... appengine/findit/common/test/time_util_test.py:60: self.assertEqual(tz.utcoffset, timedelta(minutes=480)) On 2016/10/22 00:39:16, stgao wrote: > +chanli: is this correct? I remembered you worked on some similar code? I just found out I can use 'TZ=UTC git log --date=local' to let git do the conversion, the timezone is no longer needed.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
Looks better now. https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... File appengine/findit/common/blame.py (right): https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/blame.py:9: author_name=None, author_email=None, author_time=None): why default to None? An't they required? not available for local checkout? https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... File appengine/findit/common/change_log.py (right): https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/change_log.py:32: reverted_revision): Is code-review-url and reverted_revision always available? Should the change in the file reverted? https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... File appengine/findit/common/git_repository.py (right): https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/git_repository.py:46: def http_client(self): If we can't set the http_client, why it should default to None in constructor? https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... File appengine/findit/common/local_git_parsers.py (right): https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_parsers.py:83: # pylint:disable=W Why disable all Warnings? https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_parsers.py:88: for line in output.splitlines(): Can we have an outer while loop to go through all regions and an inter while loop to parse a single region as a whole? If this works, can we do the same for other parsers? https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_parsers.py:164: # pylint:disable=W same here. https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... File appengine/findit/common/repo_util.py (right): https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/repo_util.py:5: import json is it used? https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/repo_util.py:7: import urllib2 is it used?
https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... File appengine/findit/common/blame.py (right): https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/blame.py:9: author_name=None, author_email=None, author_time=None): On 2016/10/25 23:01:33, stgao wrote: > why default to None? An't they required? not available for local checkout? They are available for local checkout. This is just for convenience to parse it from git blame output. Deleted since not used anymore. https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... File appengine/findit/common/change_log.py (right): https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/change_log.py:32: reverted_revision): On 2016/10/25 23:01:33, stgao wrote: > Is code-review-url and reverted_revision always available? Should the change in > the file reverted? Oops, shouldn't delete all of them. https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... File appengine/findit/common/git_repository.py (right): https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/git_repository.py:46: def http_client(self): On 2016/10/25 23:01:33, stgao wrote: > If we can't set the http_client, why it should default to None in constructor? This would involve some refactoring since the order of repo_url and http_client needs to be changed and so do those constructed repositories in various places, will refactor this in another cl, added a TODO. https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... File appengine/findit/common/local_git_parsers.py (right): https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_parsers.py:83: # pylint:disable=W On 2016/10/25 23:01:33, stgao wrote: > Why disable all Warnings? Done. https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_parsers.py:88: for line in output.splitlines(): On 2016/10/25 23:01:33, stgao wrote: > Can we have an outer while loop to go through all regions and an inter while > loop to parse a single region as a whole? > > If this works, can we do the same for other parsers? Done. Since the author-* information only shows at the first time a revision appears. I haven't changed the logic a lot but did get the Region parsed as a whole. The Changelogs parser already did that. https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_parsers.py:164: # pylint:disable=W On 2016/10/25 23:01:33, stgao wrote: > same here. Done. https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... File appengine/findit/common/repo_util.py (right): https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/repo_util.py:5: import json On 2016/10/25 23:01:33, stgao wrote: > is it used? Done. https://codereview.chromium.org/2435863003/diff/160001/appengine/findit/commo... appengine/findit/common/repo_util.py:7: import urllib2 On 2016/10/25 23:01:33, stgao wrote: > is it used? Done.
https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... File appengine/findit/common/repo_util.py (right): https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/repo_util.py:37: lines = message.strip().split('\n')[-5:] On 2016/10/21 01:07:42, sharu jiang wrote: > On 2016/10/20 23:31:32, lijeffrey wrote: > > move this to a constant > > Cannot think of a good name except a extremely long one: > > COMMIT_POSITION_CODE_REVIEW_URL_PARSING_START_LINE START_OF_CR_COMMIT_POSITION (or expand "CR" if preferred)
Hi, I added back the TimeZoneInfo since the --date:<format> will be ignored by --porcelain and I cannot find a way around this, so we have to change time zone by ourselves for git blame --porcelain/--incremental https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... File appengine/findit/common/repo_util.py (right): https://codereview.chromium.org/2435863003/diff/20001/appengine/findit/common... appengine/findit/common/repo_util.py:37: lines = message.strip().split('\n')[-5:] On 2016/10/28 18:15:36, wrengr wrote: > On 2016/10/21 01:07:42, sharu jiang wrote: > > On 2016/10/20 23:31:32, lijeffrey wrote: > > > move this to a constant > > > > Cannot think of a good name except a extremely long one: > > > > COMMIT_POSITION_CODE_REVIEW_URL_PARSING_START_LINE > > START_OF_CR_COMMIT_POSITION (or expand "CR" if preferred) Sounds good :)
Patchset #9 (id:240001) has been deleted
Patchset #9 (id:260001) has been deleted
https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/commo... File appengine/findit/common/time_util.py (right): https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/commo... appengine/findit/common/time_util.py:71: class TimeZoneInfo(object): We already have the code to handle this in https://chromium.googlesource.com/infra/infra/+/master/appengine/findit/lib/g... Can we just refactor and reuse? https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/local_git_parsers.py (right): https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_parsers.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. Why this file should be in lib/gitiles? It is not related to gitiles at all, right? https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/repo_util.py (right): https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... appengine/findit/lib/gitiles/repo_util.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. This module is not related to operation on a repo. It is more to parse the commit message for metadata instead.
Patchset #10 (id:300001) has been deleted
Patchset #10 (id:320001) has been deleted
Patchset #10 (id:340001) has been deleted
Patchset #10 (id:360001) has been deleted
https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/local_git_parsers.py (right): https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_parsers.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/02 02:04:20, stgao (slow on Monday) wrote: > Why this file should be in lib/gitiles? It is not related to gitiles at all, > right? Fwiw, it makes sense to me for the file to live in the same directory as the gitiles stuff: it doesn't pull in any dependencies, it's generally useful, it makes sense as part of the general API along with Blame, Region, etc (which also aren't Gitiles-specific). Perhaps the directory should be renamed to ./lib/git or something? https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_parsers.py:11: from common import time_util Things in ./lib shouldn't depend on things in ./common Given stgao's comment about having other code for this and wanting to refactor/combine the implementations, it probably makes sense to add ./lib/time_util (or similar) and move the files there. https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/repo_util.py (right): https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... appengine/findit/lib/gitiles/repo_util.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/02 02:04:20, stgao (slow on Monday) wrote: > This module is not related to operation on a repo. > It is more to parse the commit message for metadata instead. I'm not sure I follow? I agree that it's not an operation on the repo itself, but not everything that lives in ./lib/gitiles is an operation on a repo (nor should it be). Still, it does look a bit strange for being here. This seems more related to CR stuff than to repo stuff. Should be put into ./lib/code_review (or similar) along with rietveldt.py etc, ne?
https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/repo_util.py (right): https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... appengine/findit/lib/gitiles/repo_util.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/03 17:09:01, wrengr wrote: > On 2016/11/02 02:04:20, stgao (slow on Monday) wrote: > > This module is not related to operation on a repo. > > It is more to parse the commit message for metadata instead. > > I'm not sure I follow? I agree that it's not an operation on the repo itself, > but not everything that lives in ./lib/gitiles is an operation on a repo (nor > should it be). > > Still, it does look a bit strange for being here. This seems more related to CR > stuff than to repo stuff. Should be put into ./lib/code_review (or similar) > along with rietveldt.py etc, ne? I'm fine with it being with git-related modules, but the file name could be improved a little bit (repo_util.py sounds not ideal to me).
Patchset #10 (id:380001) has been deleted
Patchset #10 (id:390001) has been deleted
Patchset #10 (id:410001) has been deleted
Patchset #10 (id:430001) has been deleted
Patchset #10 (id:450001) has been deleted
This cl grows big because I moved time_util from common/ to lib/ after refactor. I also move all local_checkout related stuff to util_scripts/git_checkout or the app engine will throw errors because it does not allow any writing to file system. Please focus on files under lib/ and util_script/git_checkout/, thanks :) https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/commo... File appengine/findit/common/time_util.py (right): https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/commo... appengine/findit/common/time_util.py:71: class TimeZoneInfo(object): On 2016/11/02 02:04:20, stgao (slow on Monday) wrote: > We already have the code to handle this in > https://chromium.googlesource.com/infra/infra/+/master/appengine/findit/lib/g... > > Can we just refactor and reuse? Sure, I can refactor that code to use this TimeZoneInfo class. https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/local_git_parsers.py (right): https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_parsers.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/03 17:09:01, wrengr wrote: > On 2016/11/02 02:04:20, stgao (slow on Monday) wrote: > > Why this file should be in lib/gitiles? It is not related to gitiles at all, > > right? > > Fwiw, it makes sense to me for the file to live in the same directory as the > gitiles stuff: it doesn't pull in any dependencies, it's generally useful, it > makes sense as part of the general API along with Blame, Region, etc (which also > aren't Gitiles-specific). Perhaps the directory should be renamed to ./lib/git > or something? I found out that app engine does not allow any writing to the file system, hence all those local_* stuff should only be used by local scrips. I moved those local_* to util_script/git_checkout/ https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_parsers.py:11: from common import time_util On 2016/11/03 17:09:01, wrengr wrote: > Things in ./lib shouldn't depend on things in ./common > > Given stgao's comment about having other code for this and wanting to > refactor/combine the implementations, it probably makes sense to add > ./lib/time_util (or similar) and move the files there. Move the time_util to lib/ https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/repo_util.py (right): https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... appengine/findit/lib/gitiles/repo_util.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/02 02:04:20, stgao (slow on Monday) wrote: > This module is not related to operation on a repo. > It is more to parse the commit message for metadata instead. It is parsing commit messages from a git repo :) changed the name to util.py under lib/ https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... appengine/findit/lib/gitiles/repo_util.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/03 17:28:40, stgao (slow on Monday) wrote: > On 2016/11/03 17:09:01, wrengr wrote: > > On 2016/11/02 02:04:20, stgao (slow on Monday) wrote: > > > This module is not related to operation on a repo. > > > It is more to parse the commit message for metadata instead. > > > > I'm not sure I follow? I agree that it's not an operation on the repo itself, > > but not everything that lives in ./lib/gitiles is an operation on a repo (nor > > should it be). > > > > Still, it does look a bit strange for being here. This seems more related to > CR > > stuff than to repo stuff. Should be put into ./lib/code_review (or similar) > > along with rietveldt.py etc, ne? > > I'm fine with it being with git-related modules, but the file name could be > improved a little bit (repo_util.py sounds not ideal to me). How about code_review_util.py under the lib/?
https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/local_git_parsers.py (right): https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_parsers.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/03 20:59:48, Sharu Jiang wrote: > On 2016/11/03 17:09:01, wrengr wrote: > > On 2016/11/02 02:04:20, stgao (slow on Monday) wrote: > > > Why this file should be in lib/gitiles? It is not related to gitiles at all, > > > right? > > > > Fwiw, it makes sense to me for the file to live in the same directory as the > > gitiles stuff: it doesn't pull in any dependencies, it's generally useful, it > > makes sense as part of the general API along with Blame, Region, etc (which > also > > aren't Gitiles-specific). Perhaps the directory should be renamed to ./lib/git > > or something? > > I found out that app engine does not allow any writing to the file system, hence > all those local_* stuff should only be used by local scrips. I moved those > local_* to util_script/git_checkout/ I don't think util_script is the right place for that. Except for the cache decorator, the lib/gitiles (and lib/* more generally) stuff is entirely independent of AppEngine. So we shouldn't let AppEngine dictate what goes where in there. Putting a comment/docstring in the file indicating that it cannot be used with AppEngine is sufficient.
On 2016/11/03 20:59:48, Sharu Jiang wrote: > How about code_review_util.py under the lib/? In general I dislike the foo_util.py naming scheme. The name "util" (like "common") doesn't give any information about what it does, and makes it too easy to just put everything and the kitchen sink in there. Since we'll want to have ./lib/code_review/ anyways, just make that directory and call this file something like ./lib/code_review/commit_message_parser.py
Patchset #10 (id:470001) has been deleted
Patchset #10 (id:490001) has been deleted
Patchset #11 (id:530001) has been deleted
On 2016/11/03 21:20:25, wrengr wrote: > On 2016/11/03 20:59:48, Sharu Jiang wrote: > > How about code_review_util.py under the lib/? > > In general I dislike the foo_util.py naming scheme. The name "util" (like > "common") doesn't give any information about what it does, and makes it too easy > to just put everything and the kitchen sink in there. Since we'll want to have > ./lib/code_review/ anyways, just make that directory and call this file > something like ./lib/code_review/commit_message_parser.py The naming scheme is already widely used, if we want to refactor it, we can address them in other cls, let's use commit_util.py here to be consistent.
On 2016/11/03 21:16:09, wrengr wrote: > https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... > File appengine/findit/lib/gitiles/local_git_parsers.py (right): > > https://codereview.chromium.org/2435863003/diff/280001/appengine/findit/lib/g... > appengine/findit/lib/gitiles/local_git_parsers.py:1: # Copyright 2016 The > Chromium Authors. All rights reserved. > On 2016/11/03 20:59:48, Sharu Jiang wrote: > > On 2016/11/03 17:09:01, wrengr wrote: > > > On 2016/11/02 02:04:20, stgao (slow on Monday) wrote: > > > > Why this file should be in lib/gitiles? It is not related to gitiles at > all, > > > > right? > > > > > > Fwiw, it makes sense to me for the file to live in the same directory as the > > > gitiles stuff: it doesn't pull in any dependencies, it's generally useful, > it > > > makes sense as part of the general API along with Blame, Region, etc (which > > also > > > aren't Gitiles-specific). Perhaps the directory should be renamed to > ./lib/git > > > or something? > > > > I found out that app engine does not allow any writing to the file system, > hence > > all those local_* stuff should only be used by local scrips. I moved those > > local_* to util_script/git_checkout/ > > I don't think util_script is the right place for that. Except for the cache > decorator, the lib/gitiles (and lib/* more generally) stuff is entirely > independent of AppEngine. So we shouldn't let AppEngine dictate what goes where > in there. Putting a comment/docstring in the file indicating that it cannot be > used with AppEngine is sufficient. After discussion with stgao@, putting local_* in util_script/ because they are supposed to be only used locally, and there won't be any other project that will use this in the near future.
Patchset #13 (id:590001) has been deleted
Patchset #14 (id:630001) has been deleted
Patchset #13 (id:610001) has been deleted
Patchset #14 (id:670001) has been deleted
Patchset #13 (id:650001) has been deleted
Patchset #13 (id:690001) has been deleted
Ping.
lgtm with nits https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/local_git_parsers.py (right): https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_parsers.py:213: info = {'message':'', 'touched_files':[]} nit: spaces after colons https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_parsers.py:400: class GitSourceParser(GitParser): This seems strange. Is the intention to actually do some parsing in a future CL? If not, then why have a class for this? Could just as well use ``lambda output: return output if output else None``... https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py (right): https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:59: expected_blame.AddRegion(expected_region) Can you add a TODO(crbug.com/663445)? resolving that can be done in a future cl https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:61: blame_result = local_git_parsers.GitBlameParser()(output, should prolly cache the local_git_parsers.GitBlameParser() and local_git_parsers.GitChangeLogParser() during setUp https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:70: output = '' since output and blame_result are only used once (and are relatively short), they should probably be inlined like they are in testGetFileChangeInfo https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:77: output = 'Dummy' ditto https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:132: output = None ditto https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:237: output = None ditto https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:246: output = 'output' ditto https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:250: output = 'output' ditto
lgtm if comments are addressed. https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... File appengine/findit/util_scripts/__init__.py (right): https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/__init__.py:21: # from util_scripts, so added this util_scripts/ directory as working What's "working directory"? This sounds incorrect to me. https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/__init__.py:27: import appengine_config Why we have this here? Do you understand why it is needed for the new code structure of Predator? https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/local_git_parsers.py (right): https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_parsers.py:46: 'D': 'delete', Could we use https://chromium.googlesource.com/infra/infra/+/master/appengine/findit/lib/g... instead of string here?
Patchset #15 (id:750001) has been deleted
Patchset #16 (id:790001) has been deleted
https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... File appengine/findit/util_scripts/__init__.py (right): https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/__init__.py:21: # from util_scripts, so added this util_scripts/ directory as working On 2016/11/08 21:44:16, stgao (slow on Monday) wrote: > What's "working directory"? This sounds incorrect to me. I think the working directory here is the "current directory", which means the sys.path[0]. Is that correct? https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/__init__.py:27: import appengine_config On 2016/11/08 21:44:16, stgao (slow on Monday) wrote: > Why we have this here? Do you understand why it is needed for the new code > structure of Predator? Done. https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/local_git_parsers.py (right): https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_parsers.py:46: 'D': 'delete', On 2016/11/08 21:44:16, stgao (slow on Monday) wrote: > Could we use > https://chromium.googlesource.com/infra/infra/+/master/appengine/findit/lib/g... > instead of string here? Done. https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_parsers.py:213: info = {'message':'', 'touched_files':[]} On 2016/11/08 19:05:16, wrengr wrote: > nit: spaces after colons Done. https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_parsers.py:400: class GitSourceParser(GitParser): On 2016/11/08 19:05:16, wrengr wrote: > This seems strange. Is the intention to actually do some parsing in a future CL? > If not, then why have a class for this? Could just as well use ``lambda output: > return output if output else None``... No parsing I can think of for now, this is just to keep all the git operation (changelog, changelogs, blame, diff, source) covered. But, I can remove it since it's not very useful to have this Parser. https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py (right): https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:59: expected_blame.AddRegion(expected_region) On 2016/11/08 19:05:16, wrengr wrote: > Can you add a TODO(crbug.com/663445)? resolving that can be done in a future cl Done. https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:61: blame_result = local_git_parsers.GitBlameParser()(output, On 2016/11/08 19:05:16, wrengr wrote: > should prolly cache the local_git_parsers.GitBlameParser() and > local_git_parsers.GitChangeLogParser() during setUp Done. https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:70: output = '' On 2016/11/08 19:05:16, wrengr wrote: > since output and blame_result are only used once (and are relatively short), > they should probably be inlined like they are in testGetFileChangeInfo Done. https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:77: output = 'Dummy' On 2016/11/08 19:05:16, wrengr wrote: > ditto Done. https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:132: output = None On 2016/11/08 19:05:16, wrengr wrote: > ditto Done. https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:237: output = None On 2016/11/08 19:05:16, wrengr wrote: > ditto Done. https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:246: output = 'output' On 2016/11/08 19:05:16, wrengr wrote: > ditto Done. https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py:250: output = 'output' On 2016/11/08 19:05:16, wrengr wrote: > ditto Done.
https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... File appengine/findit/util_scripts/__init__.py (right): https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/__init__.py:21: # from util_scripts, so added this util_scripts/ directory as working On 2016/11/10 22:28:37, Sharu Jiang wrote: > On 2016/11/08 21:44:16, stgao (slow on Monday) wrote: > > What's "working directory"? This sounds incorrect to me. > > I think the working directory here is the "current directory", which means the > sys.path[0]. Is that correct? I'm confused with the terms here. But you may want to search around for the difference between sys.path (or PYTHONPATH) and "current working directory". The wording here is still misleading though. We need to update before commit.
https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... File appengine/findit/util_scripts/__init__.py (right): https://codereview.chromium.org/2435863003/diff/730001/appengine/findit/util_... appengine/findit/util_scripts/__init__.py:21: # from util_scripts, so added this util_scripts/ directory as working On 2016/11/10 22:33:47, stgao (slow on Monday) wrote: > On 2016/11/10 22:28:37, Sharu Jiang wrote: > > On 2016/11/08 21:44:16, stgao (slow on Monday) wrote: > > > What's "working directory"? This sounds incorrect to me. > > > > I think the working directory here is the "current directory", which means the > > sys.path[0]. Is that correct? > > I'm confused with the terms here. But you may want to search around for the > difference between sys.path (or PYTHONPATH) and "current working directory". > > The wording here is still misleading though. We need to update before commit. Done.
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/2435863003/#ps810001 (title: "Fix nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/326e0bd99f482510)
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/2435863003/#ps830001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/326e4668ab132710)
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/2435863003/#ps850001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Findit] Add local git parsers. BUG=605369 ========== to ========== [Findit] Add local git parsers. BUG=605369 Committed: https://chromium.googlesource.com/infra/infra/+/b8b4590396043079125412a7e3715... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:850001) as https://chromium.googlesource.com/infra/infra/+/b8b4590396043079125412a7e3715...
Message was sent while issue was closed.
Patchset #17 (id:830001) has been deleted |
