Only show landmines output if the environment says so.

Only show landmines output if the environment says so.

R=maruel@chromium.org
BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167834

[iannucci, 2012-11-14 09:06:42]

This makes landmines.py quiet on the dev's machine.

[M-A Ruel, 2012-11-14 13:03:36]

https://chromiumcodereview.appspot.com/11369238/diff/3001/build/landmines.py
File build/landmines.py (right):

https://chromiumcodereview.appspot.com/11369238/diff/3001/build/landmines.py#...
build/landmines.py:39: if 'LANDMINES_VERBOSE' in os.environ:
I prefer when you use a --verbose flag and
default=bool(try_int(os.environ.get('LANDMINES_VERBOSE', '0')))
with try_int be a safe int conversion function. Otherwise, how someone is
supposed to figure out how to do this without reading the whole code?

This means using optparse though but I think it's worth in that case.

[iannucci, 2012-11-14 22:42:14]

Ok, added optparse to catch -v/--verbose as well.

[M-A Ruel, 2012-11-14 23:12:01]

lgtm with optional style nits.

By habit, I would have used
logging.basicConfig(level=logging.DEBUG if options.verbose else logging.ERROR)
 and then used:
if logging.getLogger().isEnabledFor(logging.INFO):
  print 'foo'

https://chromiumcodereview.appspot.com/11369238/diff/1002/build/landmines.py
File build/landmines.py (right):

https://chromiumcodereview.appspot.com/11369238/diff/1002/build/landmines.py#...
build/landmines.py:173: def GetOptions():
I don't think it's useful to have a function just for that for a file this
short. I'd personally merge that code in main().

https://chromiumcodereview.appspot.com/11369238/diff/1002/build/landmines.py#...
build/landmines.py:175: parser.add_option('-v', '--verbose',
action='store_true', default=False,
I prefer default='LANDMINES_VERBOSE' in os.environ, have line 30 with VERBOSE =
False and have 'default: %default' in the help. But I don't mind much.

https://chromiumcodereview.appspot.com/11369238/diff/1002/build/landmines.py#...
build/landmines.py:192: 
FYI, I usually put a main() where I do all the argument parsing and sanitisation
and then call the "main function", which I usually name after the name of the
script name, to do the actual work. This gives a better separation of "input
sanitization vs work".

In your case, you have the "work" function call the sanitization, I usually do
the reverse.

That's fyi, I don't mind.

[iannucci, 2012-11-14 23:30:11]

On 2012/11/14 23:12:01, Marc-Antoine Ruel wrote:
> lgtm with optional style nits.
> 
> By habit, I would have used
> logging.basicConfig(level=logging.DEBUG if options.verbose else logging.ERROR)
>  and then used:
> if logging.getLogger().isEnabledFor(logging.INFO):
>   print 'foo'
> 
> https://chromiumcodereview.appspot.com/11369238/diff/1002/build/landmines.py
> File build/landmines.py (right):
> 
>
https://chromiumcodereview.appspot.com/11369238/diff/1002/build/landmines.py#...
> build/landmines.py:173: def GetOptions():
> I don't think it's useful to have a function just for that for a file this
> short. I'd personally merge that code in main().
> 
>
https://chromiumcodereview.appspot.com/11369238/diff/1002/build/landmines.py#...
> build/landmines.py:175: parser.add_option('-v', '--verbose',
> action='store_true', default=False,
> I prefer default='LANDMINES_VERBOSE' in os.environ, have line 30 with VERBOSE
=
> False and have 'default: %default' in the help. But I don't mind much.
> 
>
https://chromiumcodereview.appspot.com/11369238/diff/1002/build/landmines.py#...
> build/landmines.py:192: 
> FYI, I usually put a main() where I do all the argument parsing and
sanitisation
> and then call the "main function", which I usually name after the name of the
> script name, to do the actual work. This gives a better separation of "input
> sanitization vs work".
> 
> In your case, you have the "work" function call the sanitization, I usually do
> the reverse.
> 
> That's fyi, I don't mind.

Good feedback, I appreciate it. If this will make the code more harmonious to
the codebase then that's good :).

That said, this cl is getting large :D. Does this refactor feel better?

[M-A Ruel, 2012-11-14 23:34:17]

yes, I prefer a lot.

lgtm

[commit-bot: I haz the power, 2012-11-14 23:35:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/iannucci@chromium.org/11369238/4003

[commit-bot: I haz the power, 2012-11-15 02:53:08]

Change committed as 167834