Ubuntu CI Engine

Merge lp://qastaging/~doanac/uci-engine/lander-cleanups-4 into lp://qastaging/uci-engine

  1. lander-cleanups-4
  2. Merge into trunk
Proposed by Andy Doan
Status: Merged
Approved by: Francis Ginther
Approved revision: 442
Merged at revision: 450
Proposed branch: lp://qastaging/~doanac/uci-engine/lander-cleanups-4
Merge into: lp://qastaging/uci-engine
Prerequisite: lp://qastaging/~doanac/uci-engine/lander-cleanups-3
Diff against target: 102 lines (+33/-25)
3 files modified
lander/bin/lander_service_wrapper.py (+2/-14)
lander/lander/__init__.py (+28/-0)
lander/lander/run_worker.py (+3/-11)
To merge this branch: bzr merge lp://qastaging/~doanac/uci-engine/lander-cleanups-4
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Francis Ginther Approve
Vincent Ladeuil (community) Approve
Review via email: mp+216243@code.qastaging.launchpad.net

Commit message

lander: use common logging setup

We have 2 scripts needing the same logging setup and will soon
have a 3rd. This gets the lander using a common mechanism.

Description of the change

lander: use common logging setup

We have 2 scripts needing the same logging setup and will soon
have a 3rd. This gets the lander using a common mechanism.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:441
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/511/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/511/rebuild

review: Approve (continuous-integration)
Revision history for this message
Vincent Ladeuil (vila) wrote :

47 +def _init():

s/_init/init_logging/ please ;)

I like the pattern of having high-level components setting up the loggers for the modules they use. 'cli' does something similar and tests badly need that too to reduce the noise in the output. Let's have more of that !

review: Approve
Revision history for this message
Andy Doan (doanac) wrote :

On 04/23/2014 11:27 AM, Vincent Ladeuil wrote:
> 47 +def _init():
>
> s/_init/init_logging/ please;)

really? This is a private function just for the module. I'm not sure we
want to give the impression its intended to be used anywhere else?

Revision history for this message
Francis Ginther (fginther) wrote :

Perhaps vila meant 's/_init/_init_logging/'?

+1 on the refactoring.

review: Approve
Revision history for this message
Vincent Ladeuil (vila) wrote :

> On 04/23/2014 11:27 AM, Vincent Ladeuil wrote:
> > 47 +def _init():
> >
> > s/_init/init_logging/ please;)
>
> really? This is a private function just for the module. I'm not sure we
> want to give the impression its intended to be used anywhere else?

Private in python are... a convention that anybody is perfectly happy to ignore most of the time so I don't rely on it.

Specifically here I'm asking for a more meaningful name (+ logging). If we generalize the pattern some modules will have that function public, so we may as well make it public for all modules.

lp://qastaging/~doanac/uci-engine/lander-cleanups-4 updated
442. By Andy Doan

rename init function

Revision history for this message
Andy Doan (doanac) wrote :

On 04/24/2014 01:25 AM, Vincent Ladeuil wrote:
> Private in python are... a convention that anybody is perfectly happy to ignore most of the time so I don't rely on it.

While a person can choose to ignore convention, I don't think that's
reason enough to not give hints.

> Specifically here I'm asking for a more meaningful name (+ logging). If we generalize the pattern some modules will have that function public, so we may as well make it public for all modules.

I've renamed to _init_logging, but even if this became used everywhere.
Nobody will ever call _init_logging - you call get_logger(<name>).
get_logger knows if _init_logging needs to be called or not.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:442
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/534/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/534/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches