Replace Logger.{Level,SetLevel} with DynamicLevel #180
Conversation
jcorbin
force-pushed
the
drop_logger_level
branch
from
f7536a3 to
92c5828
Compare
jcorbin
changed the title
|
Rebased, ready for review |
| @@ -35,18 +35,19 @@ type errorResponse struct { | |||
| } | |||
|
|
|||
| type levelHandler struct { | |||
| logger Logger | |||
| lvl AtomicLevel | |||
There was a problem hiding this comment.
Why not just add ServeHTTP to the AtomicLevel type? It doesn't add any overhead if it's not used, and it removes one type and wrapper from this now-crowded package.
| @@ -105,3 +121,41 @@ func (l *Level) UnmarshalText(text []byte) error { | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| // Enabled return true if the given level is at or above this level. | |||
| // | ||
| // The value's SetLevel() method may be called later to change the enabled | ||
| // logging level of all loggers that were passed the value (either explicitly, | ||
| // or by creating sub-loggers with Logger.With). |
There was a problem hiding this comment.
nice - this is a much clearer explanation than the parent/child terminology I was using.
| @@ -37,37 +33,25 @@ type Meta struct { | |||
| Hooks []Hook | |||
| Output WriteSyncer | |||
| ErrorOutput WriteSyncer | |||
|
|
|||
| lvl *atomic.Int32 | |||
| LevelEnabler | |||
There was a problem hiding this comment.
Can we keep embeds at the top, separated from the other fields with a blank line?
|
Great - appreciate the quick turnaround after a delayed review. Feel free to merge after resolving the conflict. |
jcorbin
force-pushed
the
drop_logger_level
branch
from
c64b035 to
1410575
Compare
|
A quick question on this PR. You seem to have removed the Level access on Logger object. How will a user know what log level is set on the created/passed zap.Logger object? |
|
@anuptalwalkar the idea now is that there isn't necessary a "the level of a logger object" concept; I'd respond to your question of "how?" with a clarifying "why?":
If those don't meet your needs, can you describe your use case a little more? (maybe in a new issue?) |
|
I see. my concerns were around setting up the Logger correctly. So now I can add |
|
Nevermind actually. I understand now. I have updated my changes as well uber-go/fx#81 |
* make Levels be "just" Options; simplify/extensify meta enabled layer * drop SetLevel/Level Logger API * add AtomicLevel option for changeable levels
Depends on #172, supercedes #173.
After this PR, the only part of #166 left is the CheckedMessage composition work.