Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2657973006:
Make it possible to override more than one package (Closed)
|
Created:
3 years, 10 months ago by Paweł Hajdan Jr. Modified:
3 years, 10 months ago CC:
chromium-reviews, infra-reviews+recipes-py_chromium.org Target Ref:
refs/heads/master Project:
recipe_engine Visibility:
Public. |
DescriptionMake it possible to override more than one package
See https://groups.google.com/a/chromium.org/d/msg/infra-dev/8Hryqf4HzaQ/x5Nb0RvDBgAJ
for context.
BUG=none
Review-Url: https://codereview.chromium.org/2657973006
Committed: https://github.com/luci/recipes-py/commit/30a672d26b45289947783932d8062fc70dfb1611
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix #Messages
Total messages: 19 (12 generated)
The CQ bit was checked by phajdan.jr@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: This issue passed the CQ dry run.
phajdan.jr@chromium.org changed reviewers: + dnj@chromium.org, dpranke@chromium.org, iannucci@chromium.org
I know nothing about this code, so I'm the wrong reviewer here :).
lgtm w/ comment https://codereview.chromium.org/2657973006/diff/1/recipe_engine/package.py File recipe_engine/package.py (right): https://codereview.chromium.org/2657973006/diff/1/recipe_engine/package.py#ne... recipe_engine/package.py:176: return (self == other) the equality operator overload in this file is a bit surprising :/. it might make more sense to explicitly define this on GitRepoSpec instead of relying on it's overload of __eq__ in the base class. Even if you just return self == other in GitRepoSpec, it's better than going up and down the class hierarchy multiple times. (So: make this method raise NotImplementedError in the base class here, and implement in all subclasses)
The CQ bit was checked by phajdan.jr@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2657973006/diff/1/recipe_engine/package.py File recipe_engine/package.py (right): https://codereview.chromium.org/2657973006/diff/1/recipe_engine/package.py#ne... recipe_engine/package.py:176: return (self == other) On 2017/01/30 17:53:20, iannucci wrote: > the equality operator overload in this file is a bit surprising :/. it might > make more sense to explicitly define this on GitRepoSpec instead of relying on > it's overload of __eq__ in the base class. Even if you just return self == other > in GitRepoSpec, it's better than going up and down the class hierarchy multiple > times. > > (So: make this method raise NotImplementedError in the base class here, and > implement in all subclasses) Done.
lgtm, feel free to land
The CQ bit was checked by phajdan.jr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485805321886870,
"parent_rev": "b120624b72ec851b0d9f2accccf58e525555be25", "commit_rev":
"30a672d26b45289947783932d8062fc70dfb1611"}
Message was sent while issue was closed.
Description was changed from ========== Make it possible to override more than one package See https://groups.google.com/a/chromium.org/d/msg/infra-dev/8Hryqf4HzaQ/x5Nb0RvD... for context. BUG=none ========== to ========== Make it possible to override more than one package See https://groups.google.com/a/chromium.org/d/msg/infra-dev/8Hryqf4HzaQ/x5Nb0RvD... for context. BUG=none Review-Url: https://codereview.chromium.org/2657973006 Committed: https://github.com/luci/recipes-py/commit/30a672d26b45289947783932d8062fc70df... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://github.com/luci/recipes-py/commit/30a672d26b45289947783932d8062fc70df... |
