[Predator] Move time_util from common/ to lib/, split code review related part to code_review_util

[Predator] Move time_util from common/ to lib/, split code review related part in gitiles_repository to code_review_util

Also refactor the time zone part in gitile repository to use TimeZoneInfo in time_util.

BUG=660474
Committed: https://chromium.googlesource.com/infra/infra/+/7db2966deb7a599377da34981323f0790c4d58d3

[Sharu Jiang, 2016-11-03 21:57:45]

Description was changed from

==========
Move time_util from common/ to lib/

BUG=
==========

to

==========
[Predator] Move time_util from common/ to lib/, split code review parsing part
to code_review_util

BUG=660474
==========

[Sharu Jiang, 2016-11-03 21:57:45]

katesonia@chromium.org changed reviewers:
+ stgao@chromium.org, wrengr@chromium.org

[Sharu Jiang, 2016-11-03 21:59:56]

Description was changed from

==========
[Predator] Move time_util from common/ to lib/, split code review parsing part
to code_review_util

BUG=660474
==========

to

==========
[Predator] Move time_util from common/ to lib/, split code review related part
in gitiles_repository to code_review_util

BUG=660474
==========

[Sharu Jiang, 2016-11-03 22:01:04]

Description was changed from

==========
[Predator] Move time_util from common/ to lib/, split code review related part
in gitiles_repository to code_review_util

BUG=660474
==========

to

==========
[Predator] Move time_util from common/ to lib/, split code review related part
in gitiles_repository to code_review_util

Also refactor the time zone part in gitile repository to use TimeZoneInfo in
time_util.

BUG=660474
==========

[Sharu Jiang, 2016-11-03 22:01:17]

PTAL :)

[stgao, 2016-11-03 22:14:05]

lgtm after comments addressed.

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/code_r...
File appengine/findit/lib/code_review_util.py (right):

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/code_r...
appengine/findit/lib/code_review_util.py:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
How about commit_util.py  ?

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/time_u...
File appengine/findit/lib/time_util.py (right):

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/time_u...
appengine/findit/lib/time_util.py:71: class TimeZoneInfo(object):
Can we make this a simple function like the other above?

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/time_u...
appengine/findit/lib/time_util.py:101: datetime(2016, 9, 1, 18, 0, 0) in utc
time.
Is this true? Shouldn't it be datetime(2016, 9, 1, 2, 0, 0) in UTC for
datetime(2016, 9, 1, 10, 0, 0) at timezone +8?

https://codereview.chromium.org/2480593002/diff/20001/appengine/findit/crash/...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2480593002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:120: def run(self, *_args, **_kargs):
nit: _kwargs

[wrengr, 2016-11-03 22:17:59]

lgtm, with nit about file name

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/crash/cras...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/crash/cras...
appengine/findit/crash/crash_pipeline.py:110: def run(self, *_args, **_kargs):
Hrm, this should've been fixed in the landed version of 2455053004 (along with
adding a docstring explaining why the arguments are discarded). Can you rebase?

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/crash/cras...
appengine/findit/crash/crash_pipeline.py:155: def run(self, *_args, **_kargs):
ditto

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/crash/cras...
appengine/findit/crash/crash_pipeline.py:188: def run(self, *_args, **_kargs): #
pragma: no cover
ditto

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/code_r...
File appengine/findit/lib/code_review_util.py (right):

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/code_r...
appengine/findit/lib/code_review_util.py:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
I think this file should be renamed/moved to
./lib/code_review/commit_message_parser.py  or similar, as mentioned in the
related CL. We can always do that move in the future, but might as well do it
now.

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/time_u...
File appengine/findit/lib/time_util.py (right):

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/time_u...
appengine/findit/lib/time_util.py:71: class TimeZoneInfo(object):
On 2016/11/03 22:14:05, stgao (slow on Monday) wrote:
> Can we make this a simple function like the other above?

Why?

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right):

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:12: 
Delete newline?

[stgao, 2016-11-03 23:04:52]

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/code_r...
File appengine/findit/lib/code_review_util.py (right):

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/code_r...
appengine/findit/lib/code_review_util.py:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/11/03 22:17:58, wrengr wrote:
> I think this file should be renamed/moved to
> ./lib/code_review/commit_message_parser.py  or similar, as mentioned in the
> related CL. We can always do that move in the future, but might as well do it
> now.

I don't think this file is related to code review, but it is to extract metadata
from the commit message, which includes the code review url. It's still OK to
leave it with the git related stuff.

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/time_u...
File appengine/findit/lib/time_util.py (right):

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/time_u...
appengine/findit/lib/time_util.py:71: class TimeZoneInfo(object):
On 2016/11/03 22:17:58, wrengr wrote:
> On 2016/11/03 22:14:05, stgao (slow on Monday) wrote:
> > Can we make this a simple function like the other above?
> 
> Why?

The current version of it looks simpler to me:
https://chromium.googlesource.com/infra/infra/+/master/appengine/findit/lib/g...

[Sharu Jiang, 2016-11-04 01:01:35]

Patchset #3 (id:40001) has been deleted

[Sharu Jiang, 2016-11-04 01:02:35]

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/crash/cras...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/crash/cras...
appengine/findit/crash/crash_pipeline.py:110: def run(self, *_args, **_kargs):
On 2016/11/03 22:17:58, wrengr wrote:
> Hrm, this should've been fixed in the landed version of 2455053004 (along with
> adding a docstring explaining why the arguments are discarded). Can you
rebase?

Done.

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/crash/cras...
appengine/findit/crash/crash_pipeline.py:155: def run(self, *_args, **_kargs):
On 2016/11/03 22:17:58, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/crash/cras...
appengine/findit/crash/crash_pipeline.py:188: def run(self, *_args, **_kargs): #
pragma: no cover
On 2016/11/03 22:17:58, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/code_r...
File appengine/findit/lib/code_review_util.py (right):

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/code_r...
appengine/findit/lib/code_review_util.py:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/11/03 23:04:52, stgao (slow on Monday) wrote:
> On 2016/11/03 22:17:58, wrengr wrote:
> > I think this file should be renamed/moved to
> > ./lib/code_review/commit_message_parser.py  or similar, as mentioned in the
> > related CL. We can always do that move in the future, but might as well do
it
> > now.
> 
> I don't think this file is related to code review, but it is to extract
metadata
> from the commit message, which includes the code review url. It's still OK to
> leave it with the git related stuff.

Hmm, let's rename it to commit_util.py and put it under gitiles/.

and should we also rename gitiles/ to git/? since conceptually there can be more
classes which inherit git_repository and can use commit_util to parse commit
message (e.g. local_git_checkout, though sadly it cannot be in git/ since app
engine does not allow writing to the file system.)

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/time_u...
File appengine/findit/lib/time_util.py (right):

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/time_u...
appengine/findit/lib/time_util.py:71: class TimeZoneInfo(object):
On 2016/11/03 23:04:52, stgao (slow on Monday) wrote:
> On 2016/11/03 22:17:58, wrengr wrote:
> > On 2016/11/03 22:14:05, stgao (slow on Monday) wrote:
> > > Can we make this a simple function like the other above?
> > 
> > Why?
> 
> The current version of it looks simpler to me:
>
https://chromium.googlesource.com/infra/infra/+/master/appengine/findit/lib/g...

Actually I prefer this one, because it's more expandable, we can easily add
something like UTCToLocal method and if we want to convert multiple local times
to utc (though, we don't have this kind of use cases right now), this would be
more efficient.

Simplified this class a little bit :)

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/time_u...
appengine/findit/lib/time_util.py:101: datetime(2016, 9, 1, 18, 0, 0) in utc
time.
On 2016/11/03 22:14:05, stgao (slow on Monday) wrote:
> Is this true? Shouldn't it be datetime(2016, 9, 1, 2, 0, 0) in UTC for
> datetime(2016, 9, 1, 10, 0, 0) at timezone +8?

Oops, updated.

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right):

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:12: 
On 2016/11/03 22:17:58, wrengr wrote:
> Delete newline?

Done.

https://codereview.chromium.org/2480593002/diff/20001/appengine/findit/crash/...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2480593002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:120: def run(self, *_args, **_kargs):
On 2016/11/03 22:14:05, stgao (slow on Monday) wrote:
> nit: _kwargs

Done.

[Sharu Jiang, 2016-11-04 20:09:47]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-11-04 20:09:47]

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/2480593002/#ps80001
(title: "Rebase and fix nits.")

[commit-bot: I haz the power, 2016-11-04 20:09:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wrengr, 2016-11-04 20:24:26]

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/code_r...
File appengine/findit/lib/code_review_util.py (right):

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/code_r...
appengine/findit/lib/code_review_util.py:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/11/04 01:02:34, Sharu Jiang wrote:
> Hmm, let's rename it to commit_util.py and put it under gitiles/.

Works for me.

> and should we also rename gitiles/ to git/? since conceptually there can be
more
> classes which inherit git_repository and can use commit_util to parse commit
> message (e.g. local_git_checkout, though sadly it cannot be in git/ since app
> engine does not allow writing to the file system.)

I'm fine with renaming to git/

I don't see why local_git_checkout cannot be in git(iles)/. Yes, AppEngine can't
use it. But that just means that AppEngine shouldn't use it; that restriction
has no impact on where the code can/should live.

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/time_u...
File appengine/findit/lib/time_util.py (right):

https://codereview.chromium.org/2480593002/diff/1/appengine/findit/lib/time_u...
appengine/findit/lib/time_util.py:71: class TimeZoneInfo(object):
On 2016/11/04 01:02:34, Sharu Jiang wrote:
> On 2016/11/03 23:04:52, stgao (slow on Monday) wrote:
> > On 2016/11/03 22:17:58, wrengr wrote:
> > > On 2016/11/03 22:14:05, stgao (slow on Monday) wrote:
> > > > Can we make this a simple function like the other above?
> > > 
> > > Why?
> > 
> > The current version of it looks simpler to me:
> >
>
https://chromium.googlesource.com/infra/infra/+/master/appengine/findit/lib/g...
> 
> Actually I prefer this one, because it's more expandable, we can easily add
> something like UTCToLocal method and if we want to convert multiple local
times
> to utc (though, we don't have this kind of use cases right now), this would be
> more efficient.

That's the direction I was thinking, whence my question. Timezone stuff (like
all time stuff) gets really hairy really quickly, so it makes sense to me to try
and encapsulate all that complexity in a class.

[commit-bot: I haz the power, 2016-11-04 20:35:20]

Description was changed from

==========
[Predator] Move time_util from common/ to lib/, split code review related part
in gitiles_repository to code_review_util

Also refactor the time zone part in gitile repository to use TimeZoneInfo in
time_util.

BUG=660474
==========

to

==========
[Predator] Move time_util from common/ to lib/, split code review related part
in gitiles_repository to code_review_util

Also refactor the time zone part in gitile repository to use TimeZoneInfo in
time_util.

BUG=660474

Committed:
https://chromium.googlesource.com/infra/infra/+/7db2966deb7a599377da34981323f...
==========

[commit-bot: I haz the power, 2016-11-04 20:35:21]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/7db2966deb7a599377da34981323f...