-
Notifications
You must be signed in to change notification settings - Fork 247
Conversation
foreach (var kvp in values) | ||
{ | ||
builder.Append("\r\n"); | ||
for (var i = 0; i < level; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
builder.Append(new string('\t', level))?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know about this method, I like it 👍
foreach (var kvp in values) | ||
{ | ||
builder.Append("\r\n"); | ||
builder.Append('\t', level); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tabs might be overkill, especially if you end up with a deeply nested object. How does it look with just two or three spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow I didn't realize the default tab was like 8 spaces. That's insane!
@Tratcher this is what it looks like with 3 spaces |
|
||
namespace Microsoft.Framework.Logging.Console | ||
{ | ||
public class ConsoleLogger : ILogger | ||
{ | ||
private const int _indentation = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat nitpicky, but 3 is such an odd number (literally!). 2 or 4 are more common indentation levels...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 is nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✌️ ❓
…pport for general IEnumerables
if (formatter != null) | ||
if (state is ILoggerStructure) | ||
{ | ||
var builder = FormatLoggerStructure(new StringBuilder(), (ILoggerStructure)state, 1, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
named parameters
{ | ||
if (value is ILoggerStructure) | ||
{ | ||
builder = FormatLoggerStructure(builder, (ILoggerStructure)value, level + 1, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
named parameter for true
Just glazed over it, don't wait on me for anything though 😄 |
@@ -98,5 +111,60 @@ public IDisposable BeginScope(object state) | |||
{ | |||
return null; | |||
} | |||
|
|||
private StringBuilder FormatLoggerStructure(StringBuilder builder, ILoggerStructure structure, int level, bool bullet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chatted in person. This doesn't need to return the StringBuilder
because it's always the same instance. Just pass it around as a parameter.
{ | ||
var builder = new StringBuilder(); | ||
FormatLoggerStructure( | ||
builder: builder, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to provide a named parameter for the builder or state since their names state their purpose. General rule: If you can't decipher what a parameter to a method is, provide a named parameter. So things like true
, 1234
etc. are just values with no information and don't inform the reader of anything. Same comment for other instances in this file.
|
If the state of a log message is an
ILoggerStructure
, the console logger will display the result ofGetValues
, oneKeyValuePair
per line. It does this recursively for cases where theValue
is anILoggerStructure
or anIEnumerable<ILoggerStructure>