Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2128413002:
Make RetriableHttp to be composable (Closed)
DescriptionMake RetriableHttp to be composable
BUG=
R=sergeyberezin@google.com
Committed: https://chromium.googlesource.com/infra/infra/+/715fb10959225469c5650b96a7e002805c90253f
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add __setattr__ #
Total comments: 4
Patch Set 3 : Fix #
Messages
Total messages: 24 (8 generated)
The CQ bit was checked by tanin@chromium.org to run a CQ dry run
Dry run: 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
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
sergeyberezin@chromium.org changed reviewers: + sergeyberezin@chromium.org
Looks really good. Let's actually get __setattr__ in and it's ready to go. Thanks! https://codereview.chromium.org/2128413002/diff/1/infra_libs/httplib2_utils.py File infra_libs/httplib2_utils.py (right): https://codereview.chromium.org/2128413002/diff/1/infra_libs/httplib2_utils.p... infra_libs/httplib2_utils.py:207: return getattr(self._http, name) Let's also add __setattr__() here, e.g.: def __setattr__(self, name, value): self._http.__setattr__(name, value) https://codereview.chromium.org/2128413002/diff/1/infra_libs/test/httplib2_ut... File infra_libs/test/httplib2_utils_test.py (right): https://codereview.chromium.org/2128413002/diff/1/infra_libs/test/httplib2_ut... infra_libs/test/httplib2_utils_test.py:155: # TODO(tanin): fix delegating set attribute Let's fix it in this CL.
https://codereview.chromium.org/2128413002/diff/1/infra_libs/httplib2_utils.py File infra_libs/httplib2_utils.py (right): https://codereview.chromium.org/2128413002/diff/1/infra_libs/httplib2_utils.p... infra_libs/httplib2_utils.py:176: super(RetriableHttp, self).__init__(**kwargs) nit: kwargs is not used, let's remove it.
https://codereview.chromium.org/2128413002/diff/1/infra_libs/httplib2_utils.py File infra_libs/httplib2_utils.py (right): https://codereview.chromium.org/2128413002/diff/1/infra_libs/httplib2_utils.p... infra_libs/httplib2_utils.py:176: super(RetriableHttp, self).__init__(**kwargs) On 2016/07/07 22:44:36, Sergey Berezin wrote: > nit: kwargs is not used, let's remove it. Wait, nevermind - it's fine. I wasn't looking in the right place :-)
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2128413002/diff/1/infra_libs/httplib2_utils.py File infra_libs/httplib2_utils.py (right): https://codereview.chromium.org/2128413002/diff/1/infra_libs/httplib2_utils.p... infra_libs/httplib2_utils.py:176: super(RetriableHttp, self).__init__(**kwargs) On 2016/07/07 22:45:28, Sergey Berezin wrote: > On 2016/07/07 22:44:36, Sergey Berezin wrote: > > nit: kwargs is not used, let's remove it. > > Wait, nevermind - it's fine. I wasn't looking in the right place :-) We should remove kwargs like you suggested. It doesn't do anything. We are not inheriting from Http anymore. https://codereview.chromium.org/2128413002/diff/1/infra_libs/httplib2_utils.p... infra_libs/httplib2_utils.py:207: return getattr(self._http, name) On 2016/07/07 22:28:53, Sergey Berezin wrote: > Let's also add __setattr__() here, e.g.: > > def __setattr__(self, name, value): > self._http.__setattr__(name, value) Done.
A couple of more small comments. https://codereview.chromium.org/2128413002/diff/40001/infra_libs/httplib2_uti... File infra_libs/httplib2_utils.py (right): https://codereview.chromium.org/2128413002/diff/40001/infra_libs/httplib2_uti... infra_libs/httplib2_utils.py:209: if name == '_http': nit: I'd prefer to keep local attributes in the local instance: if name in ('_http', '_max_tries', '_retrying_statuses_fn'): https://codereview.chromium.org/2128413002/diff/40001/infra_libs/httplib2_uti... infra_libs/httplib2_utils.py:210: return super(RetriableHttp, self).__setattr__(name, value) I'd assign it to self: self.__dict__[name] = value. Also, no need to return value - __setattr__ returns None.
https://codereview.chromium.org/2128413002/diff/40001/infra_libs/httplib2_uti... File infra_libs/httplib2_utils.py (right): https://codereview.chromium.org/2128413002/diff/40001/infra_libs/httplib2_uti... infra_libs/httplib2_utils.py:209: if name == '_http': On 2016/07/07 23:37:41, Sergey Berezin wrote: > nit: I'd prefer to keep local attributes in the local instance: > > if name in ('_http', '_max_tries', '_retrying_statuses_fn'): Done. https://codereview.chromium.org/2128413002/diff/40001/infra_libs/httplib2_uti... infra_libs/httplib2_utils.py:210: return super(RetriableHttp, self).__setattr__(name, value) On 2016/07/07 23:37:41, Sergey Berezin wrote: > I'd assign it to self: self.__dict__[name] = value. > > Also, no need to return value - __setattr__ returns None. Done.
LGTM, thanks!
The CQ bit was checked by tanin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/07 23:49:15, Sergey Berezin wrote: > LGTM, thanks! Woohoo! now let's pray it passes CQ :P
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra Linux Precise 32 Tester on master.tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Precise...)
The CQ bit was checked by tanin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/08 00:17:28, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... I think master is broken. Will try to merge tomorrow instead.
Message was sent while issue was closed.
Description was changed from ========== Make RetriableHttp to be composable BUG= R=sergeyberezin@google.com ========== to ========== Make RetriableHttp to be composable BUG= R=sergeyberezin@google.com Committed: https://chromium.googlesource.com/infra/infra/+/715fb10959225469c5650b96a7e00... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/infra/infra/+/715fb10959225469c5650b96a7e00... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
