zerovm / zpm

ZeroVM Package Manager
Apache License 2.0
6 stars 11 forks source link

Separate standard output and logging information #116

Closed larsbutler closed 10 years ago

larsbutler commented 10 years ago

This branch separates standard output (from zpm & remotely executed zapps) and information which is more appropriate for logging.

Fixes https://github.com/zerovm/zpm/issues/98.

larsbutler commented 10 years ago

I forgot to update a couple of tests... I'll add those shortly.

larsbutler commented 10 years ago

@mgeisler

Since you didn't mention it in the commit message, it isn't clear to me if you noticed that the imports were sorted into groups of stdlib imports, zpmlib imports, and third-party imports before. This is similar to the grouping used by Google.

At present, I don't think we have any specific guidelines about the ordering of imports. Even within the stdlib imports, they were not sorted alphabetically--that's why I reordered them. If you would like to follow the Google standard, principally I'm not opposed to it. Thus, I would recommend that you separately propose an amendment to our contribution guidelines. For comparison, OpenStack also has rules about how to code imports: http://docs.openstack.org/developer/hacking/#imports.

Now all log output will use the same logger name, won't it?

Correct.

mgeisler commented 10 years ago

Lars Butler notifications@github.com writes:

@mgeisler

Since you didn't mention it in the commit message, it isn't clear to me if you noticed that the imports were sorted into groups of stdlib imports, zpmlib imports, and third-party imports before. This is similar to the grouping used by Google.

At present, I don't think we have any specific guidelines about the ordering of imports. Even within the stdlib imports, they were not sorted alphabetically--that's why I reordered them.

Yeah, they were not sorted in any particular way within the groups. I've probably sorted them roughly by length of the module name, but that's all.

If you would like to follow the Google standard, principally I'm not opposed to it. Thus, I would recommend that you separately propose an amendment to our contribution guidelines. For comparison, OpenStack also has rules about how to code imports: http://docs.openstack.org/developer/hacking/#imports.

I'm sure they do. I didn't intend to make this into a big discussion about rules, I merely wanted to let you know that the current ordering isn't completely random.

It seems that you didn't notice that and thus your commit message seems incomplete -- it lacks an argument for why this is a good change (remember to describe why, not just what and how).

I believe the current ordering is quite typical of Python projects and would propose to keep it.

Now all log output will use the same logger name, won't it?

Correct.

Maybe I wasn't clear -- the question was meant to imply that this is a bad thing. Loggers are normally created on a per-module basis (with a name as you did). That is why the logging library talks about trees of loggers. That way you can see where each log message originates and you can configure loggers for entire subtrees at once.

Here I think it would be better if you would call the singleton object 'out' instead of 'LOG'. That would make it easier to see that we're not talking about logging in a traditional sense, but that we're talking about writing output (to the user!) and doing so with differnet prefixes ('info', 'warning', etc).

If that is indeed the case, then I would also suggest that you redefine the template to make it usable as real output: maybe a space after the log level and ':', or maybe drop the prefix completely. Without a prefix, the use of the logging library is reduced to an if-statement that determines if a given message should be printed or not. We'll surely need such a if-statement sooner or later and the logging framework might be an okay way to implement it.

mgeisler commented 10 years ago

Hmm, stupid GitHub somehow thinks the bottom part of my mail should be elided:

Now all log output will use the same logger name, won't it?

Correct.

Maybe I wasn't clear -- the question was meant to imply that this is a bad thing. Loggers are normally created on a per-module basis (with a name as you did). That is why the logging library talks about trees of loggers. That way you can see where each log message originates and you can configure loggers for entire subtrees at once.

Here I think it would be better if you would call the singleton object out instead of LOG. That would make it easier to see that we're not talking about logging in a traditional sense, but that we're talking about writing output (to the user!) and doing so with different prefixes (info, warning, etc).

If that is indeed the case, then I would also suggest that you redefine the template to make it usable as real output: maybe a space after the log level and :, or maybe drop the prefix completely. Without a prefix, the use of the logging library is reduced to an if-statement that determines if a given message should be printed or not. We'll surely need such a if-statement sooner or later and the logging framework might be an okay way to implement it.