Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1663)

Issue 10043045: loggo: lots of minor changes

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 11 months ago by rog
Modified:
10 years, 11 months ago
Reviewers:
mp+167832
Visibility:
Public.

Description

loggo: lots of minor changes I've made quite a few changes here, mostly to make things more Go idiomatic, to add documentation, and hopefully to make things a bit cleaner and in one or two cases a little more efficient. I've added a final "f" onto all the Error, Warning, etc functions so that they are amenable to checking with govet. I've also taken out the special casing for no arguments - if the first argument is a printf-like format string, it should work as one regardless of whether there are arguments or not. If we want a more efficient version, we could add Logger.Error(msg string) function or similar. There seemed to be a general mixing of the term "logger" and "module" so I have changed things to standardise on "Logger" (as that's the type name) and changed the Module method and some documentation accordingly. I've also made renamed ConfigureLogging to ConfigureLoggers and DumpLoggingLevels to LoggerInfo and made ConfigureLoggers understand the format produced by LoggerInfo, which seems to me a fairly obvious thing to do. I've also made ConfigureLoggers more tolerant of white space. I haven't in this branch, but in a subsequent branch I am tempted to propose case insensitivity throughout - I think that's actually a benefit, as well as being more efficient. I think it would be best to standardise on a single separator too - probably semicolon. To go along with usual Go standards, I've also made functions that parse stuff return an error if the parsing fails. I think it's useful to be able to know if ConfigureLoggers is given an invalid specification, for example. I've changed the default formatting to print the time in UTC - if we're not printing the time zone, I think that's important. I'd quite like the default formatter to print millisecond resolution, but that can wait. Things I've thought of but have not done: - We could make Logger a pointer type and lose the module type. It's reasonable to say that a nil logger is equivalent to the root logger, I think and get the same ok-to-use zero Logger semantics. YMMV about that, so I left it for discussion. - We could make ReplaceDefaultWriter more general, say ReplaceWriter, because the functionality it provides (atomically replacing a writer) is not possible to get otherwise - it's more than a convenience method. Please feel free to say which things you don't like and I'll revert those changes. In general I like the package quite a bit - thanks for doing it! https://code.launchpad.net/~rogpeppe/loggo/minor-fixes/+merge/167832 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : loggo: lots of minor changes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+477 lines, -283 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M formatter.go View 2 chunks +6 lines, -6 lines 0 comments Download
M logger.go View 1 12 chunks +195 lines, -99 lines 0 comments Download
M logger_test.go View 12 chunks +219 lines, -134 lines 0 comments Download
M writer.go View 9 chunks +34 lines, -23 lines 0 comments Download
M writer_test.go View 8 chunks +21 lines, -21 lines 0 comments Download

Messages

Total messages: 1
rog
10 years, 11 months ago (2013-06-06 19:14:29 UTC) #1
Please take a look.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b