Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10693042:
first version of a logging library. (Closed)
|
Created:
8 years, 5 months ago by Siggi Cherem (dart-lang) Modified:
8 years, 5 months ago CC:
reviews_dartlang.org Visibility:
Public. |
Descriptionfirst version of a logging library.
Committed: https://code.google.com/p/dart/source/detail?r=9269
Patch Set 1 : #
Total comments: 72
Patch Set 2 : addressing cl comments #
Total comments: 6
Patch Set 3 : #
Messages
Total messages: 9 (0 generated)
mostly style comments. Overall this looks very nice! https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... File lib/logging/logging.dart (right): https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:8: * library and java.util.logging. did you mean: "such as the Closure JS Logger" https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:25: String name; should this be final? It seems like changing the name after creating the Logger will mess up the _loggers Map https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:27: /* The full name of this logger, which includes also the parent's names. */ /** https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:29: (parent == null || parent.name == '') ? name : '${parent.fullName}.$name'; If I'm understanding this correctly: every logger gets a parent, until we reach empty-string, and that one has null parent? https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:32: Logger parent; should this be final too? as well as the other fields. (except _level) https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:45: * Singleton constructor. Calling [:new Logger(name) :] will return the same nit, use backticks: `new Logger(name)` http://daringfireball.net/projects/markdown/syntax/#code https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:49: if (_loggers == null) _loggers = new Map<String, Logger>(); you could use "<Logger>{}" instead of "new Map<String, Logger>()". Personal preference though. Either works for me. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:54: String parentName; I think I would've understood this quicker if the code create the parent instead of getting the parentName and then checking later for null: Logger parent = null; String thisName; if (dot == -1) { if (name != '') parent = new Logger(''); thisName = name; } else { parent = new Logger(name.substring(0, dot)); thisName = name.substring(dot + 1); } final res = new Logger._internal(thisName, parent); https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:70: children = new Map<String, Logger>() { nit: indent 4 from Logger then ": children = " ... https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:91: if (parent != null) throw new Exception( UnsupportedOperationException https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:92: 'Cannot set level on a non-root logger when hierarchycal logging ' typo: should be "hierarchical" https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:93: 'is not enabled'); Maybe reword this message as: "Please set Logger.enableHierarchyLogging to true if you want to change the level of a non-root logger." friendly error messages are nice :) https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:99: void addHandler(LoggerHandler handler) { hmmm. This is just begging to use an event pattern :) Perhaps add a TODO, or start an Event pattern. I think the event would be named "message" or "onMessage" https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:131: bool isLoggable(Level value) => (value >= level); nit: redundant parens. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:134: void log(Level logLevel, String message) { This could use a doc comment too, like the other excellent ones in this file :) https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:186: static bool enableHierarchyLogging = false; does this need to be configurable? might be worth renaming this "enableHierarchy", since the logging part is implied by the "Logger." https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:205: class Level { implements Comparable, Hashable and add methods: int compareTo(Level other) => value - other.value; hashCode() => value; https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:212: final int value; TODO: this should be "const" when that modifier is supported. Likewise for the "name" field https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:246: bool operator ==(Level other) => this === other; do you need to define this? https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:284: this.time = time == null ? new Date.now() : time, nit, put ":" on next line and indent everything 4 spaces from LogRecord. Like this: LogRecord( this.level, this.message, this.loggerName, [time, sequenceNumber, this.exception, this.exceptionText]) : this.time = time == null ? new Date.now() : time, this.sequenceNumber = https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... File tests/lib/logging/logging_test.dart (right): https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:9: #import('../../../lib/unittest/unittest.dart'); do "package:" imports work yet? https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:14: expect(level1 == level1); use equals matcher? https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:16: expect(level1 >= level1); add lessThan, lessThanOrEqual matchers? (is there any reason to prefer matchers over direct operator calls for cases like this?) https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:24: expect(level2 > level1); another test idea: test('same value but different name not equal', () { var level1 = const Level('NOT_REAL1', 253); var level2 = const Level('NOT_REAL_EITHER', 253); expect(level2 != level1); // It's like comparing NaNs! expect(level2 < level1, isFalse); expect(level2 <= level1, isFalse); expect(level2 > level1, isFalse); expect(level2 >= level1, isFalse); }); https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:37: } another test: #import('dart:coreimpl'); // test they're comparable var map = new SplayTreeMap(); for (var level in levels) { map[level] = level; } levels = map.getValues(); for (int i = 1; i < levels.length; i++) { expect(levels[i - 1] < levels[i]); }
https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... File lib/logging/logging.dart (right): https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:12: /** A handler that process log entries in of a [Logger]. */ Grammar? I can't parse this sentence. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:16: Level _rootLevel = Level.INFO; Shouldn't this be called _defaultLevel then? https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:110: /** Remove an previously added handler. */ Remove a ... https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:123: if (enableHierarchyLogging || parent == null) { Can this be called 'hierarchicalLoggingEnabled' instead? It's a flag so this should be a verb. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:150: void finest(Strnig message) => log(Level.FINEST, message); Typo, here and elsewhere: Strnig https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:283: [time, sequenceNumber, this.exception, this.exceptionText]) : Is it really necessary to support passing in an explicit sequence number? Can you give a use case? This can violate your comment above too, about what _sequenceNumber is. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:284: this.time = time == null ? new Date.now() : time, If find the this.time= time == null scans very poorly, and I would prefer this.time = (time == null) ? ... Although I know the style here is to avoid unncessary parentheses, I think here they are needed, as this looks too close to: this.time = time = null https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... File tests/lib/logging/logging_test.dart (right): https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:16: expect(level1 >= level1); On 2012/06/29 03:47:45, John Messerly wrote: > add lessThan, lessThanOrEqual matchers? > > (is there any reason to prefer matchers over direct operator calls for cases > like this?) Matchers will give better error messages. https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:24: expect(level2 > level1); On 2012/06/29 03:47:45, John Messerly wrote: > another test idea: > > test('same value but different name not equal', () { > var level1 = const Level('NOT_REAL1', 253); > var level2 = const Level('NOT_REAL_EITHER', 253); > expect(level2 != level1); > // It's like comparing NaNs! > expect(level2 < level1, isFalse); > expect(level2 <= level1, isFalse); > expect(level2 > level1, isFalse); > expect(level2 >= level1, isFalse); > }); I wonder - if the value is the same but the names are different, shouldn't they still test as equal? https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:40: test('logger naming is hierarchycal', () { hierarchical https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:46: expect(c.parent.parent.parent.parent, isNull); Can you explain the rationale why there are hierarchical names like this?
https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... File tests/lib/logging/logging_test.dart (right): https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:46: expect(c.parent.parent.parent.parent, isNull); On 2012/06/29 16:38:25, gram wrote: > Can you explain the rationale why there are hierarchical names like this? I understand the intent I guess - it provides a way of controlling logging for a subset of the logs that are grouped by function rather than by level. But it seems quite complex, and I wonder if it would be simpler to just use a group name (i.e. just one level of hierarchy), which could be implemented more simply. For example, *nix logging systems do this (they have a log level and a log 'facility', which corresponds to a functional group).
https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... File lib/logging/logging.dart (right): https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:246: bool operator ==(Level other) => this === other; On 2012/06/29 03:47:45, John Messerly wrote: > do you need to define this? as gram mentioned later, maybe this should be "value == other.value". That seems more consistent with the other comparison methods. https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... File tests/lib/logging/logging_test.dart (right): https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:24: expect(level2 > level1); On 2012/06/29 16:38:25, gram wrote: > On 2012/06/29 03:47:45, John Messerly wrote: > > another test idea: > > > > test('same value but different name not equal', () { > > var level1 = const Level('NOT_REAL1', 253); > > var level2 = const Level('NOT_REAL_EITHER', 253); > > expect(level2 != level1); > > // It's like comparing NaNs! > > expect(level2 < level1, isFalse); > > expect(level2 <= level1, isFalse); > > expect(level2 > level1, isFalse); > > expect(level2 >= level1, isFalse); > > }); > > I wonder - if the value is the same but the names are different, shouldn't they > still test as equal? Yeah, I thought so too. But the code for the == seemed to explicitly check for ===. Whatever the behavior is, it seems like a case worth testing :) Oh! Another test: different instances (!==) but same name and value.
https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... File lib/logging/logging.dart (right): https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:8: * library and java.util.logging. On 2012/06/29 03:47:45, John Messerly wrote: > did you mean: "such as the Closure JS Logger" Done :) https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:12: /** A handler that process log entries in of a [Logger]. */ On 2012/06/29 16:38:25, gram wrote: > Grammar? I can't parse this sentence. Done (also moved further down...) https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:16: Level _rootLevel = Level.INFO; On 2012/06/29 16:38:25, gram wrote: > Shouldn't this be called _defaultLevel then? rephrased doc https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:25: String name; On 2012/06/29 03:47:45, John Messerly wrote: > should this be final? It seems like changing the name after creating the Logger > will mess up the _loggers Map Good point. Done. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:27: /* The full name of this logger, which includes also the parent's names. */ On 2012/06/29 03:47:45, John Messerly wrote: > /** Done. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:29: (parent == null || parent.name == '') ? name : '${parent.fullName}.$name'; On 2012/06/29 03:47:45, John Messerly wrote: > If I'm understanding this correctly: every logger gets a parent, until we reach > empty-string, and that one has null parent? Correct. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:32: Logger parent; On 2012/06/29 03:47:45, John Messerly wrote: > should this be final too? as well as the other fields. (except _level) Done. _handlers and children are not final so we avoid creating empty maps/lists when they are not used. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:45: * Singleton constructor. Calling [:new Logger(name) :] will return the same On 2012/06/29 03:47:45, John Messerly wrote: > nit, use backticks: `new Logger(name)` > http://daringfireball.net/projects/markdown/syntax/#code done, but [: :] is the dart doc way :) https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:49: if (_loggers == null) _loggers = new Map<String, Logger>(); On 2012/06/29 03:47:45, John Messerly wrote: > you could use "<Logger>{}" instead of "new Map<String, Logger>()". Personal > preference though. Either works for me. Done. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:54: String parentName; On 2012/06/29 03:47:45, John Messerly wrote: > I think I would've understood this quicker if the code create the parent instead > of getting the parentName and then checking later for null: > > Logger parent = null; > String thisName; > if (dot == -1) { > if (name != '') parent = new Logger(''); > thisName = name; > } else { > parent = new Logger(name.substring(0, dot)); > thisName = name.substring(dot + 1); > } > final res = new Logger._internal(thisName, parent); > I like it, very nice suggestion. Done. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:70: children = new Map<String, Logger>() { On 2012/06/29 03:47:45, John Messerly wrote: > nit: indent 4 from Logger then ": children = " ... Nice - I had forgotten, the style guide has the style that I actually liked :) https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:91: if (parent != null) throw new Exception( On 2012/06/29 03:47:45, John Messerly wrote: > UnsupportedOperationException Done https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:92: 'Cannot set level on a non-root logger when hierarchycal logging ' On 2012/06/29 03:47:45, John Messerly wrote: > typo: should be "hierarchical" Done, thanks! https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:93: 'is not enabled'); On 2012/06/29 03:47:45, John Messerly wrote: > Maybe reword this message as: > "Please set Logger.enableHierarchyLogging to true if you want to change the > level of a non-root logger." > > friendly error messages are nice :) Done. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:99: void addHandler(LoggerHandler handler) { On 2012/06/29 03:47:45, John Messerly wrote: > hmmm. This is just begging to use an event pattern :) > Perhaps add a TODO, or start an Event pattern. I think the event would be named > "message" or "onMessage" Done - added the pattern, yippie! Used 'on.record' rather than 'on.message' (since what you get are log records) https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:110: /** Remove an previously added handler. */ On 2012/06/29 16:38:25, gram wrote: > Remove a ... thx. done https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:123: if (enableHierarchyLogging || parent == null) { On 2012/06/29 16:38:25, gram wrote: > Can this be called 'hierarchicalLoggingEnabled' instead? It's a flag so this > should be a verb. Done. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:131: bool isLoggable(Level value) => (value >= level); On 2012/06/29 03:47:45, John Messerly wrote: > nit: redundant parens. I actually did this on purpose, it just looks really weird without them (the combination of => and >=): bool isLoggable(Level value) => value >= level; But let me know if you still want me to change it. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:134: void log(Level logLevel, String message) { On 2012/06/29 03:47:45, John Messerly wrote: > This could use a doc comment too, like the other excellent ones in this file :) Done. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:150: void finest(Strnig message) => log(Level.FINEST, message); On 2012/06/29 16:38:25, gram wrote: > Typo, here and elsewhere: Strnig wow! running tests in checked mode -- checked! https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:186: static bool enableHierarchyLogging = false; On 2012/06/29 03:47:45, John Messerly wrote: > does this need to be configurable? > > might be worth renaming this "enableHierarchy", since the logging part is > implied by the "Logger." Not sure - I was inclined to always have it enabled, but the CLosure JS implementation had this two modes, apparently for efficiency reasons sometimes they wanted to switch to non hierarchical. I moved the property to be top level and renamed according to Graham's suggestion https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:205: class Level { On 2012/06/29 03:47:45, John Messerly wrote: > implements Comparable, Hashable > > and add methods: > int compareTo(Level other) => value - other.value; > hashCode() => value; Done. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:212: final int value; On 2012/06/29 03:47:45, John Messerly wrote: > TODO: this should be "const" when that modifier is supported. > Likewise for the "name" field Done https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:246: bool operator ==(Level other) => this === other; On 2012/06/29 03:47:45, John Messerly wrote: > do you need to define this? gone https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:246: bool operator ==(Level other) => this === other; On 2012/06/29 17:37:13, John Messerly wrote: > On 2012/06/29 03:47:45, John Messerly wrote: > > do you need to define this? > > as gram mentioned later, maybe this should be "value == other.value". That seems > more consistent with the other comparison methods. agreed - done. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:283: [time, sequenceNumber, this.exception, this.exceptionText]) : On 2012/06/29 16:38:25, gram wrote: > Is it really necessary to support passing in an explicit sequence number? Can > you give a use case? This can violate your comment above too, about what > _sequenceNumber is. Removed from now. Just made this as similar as possible to the JS Closure implementation. https://chromiumcodereview.appspot.com/10693042/diff/2001/lib/logging/logging... lib/logging/logging.dart:284: this.time = time == null ? new Date.now() : time, On 2012/06/29 16:38:25, gram wrote: > If find the this.time= time == null scans very poorly, and I would prefer > this.time = (time == null) ? ... > > Although I know the style here is to avoid unncessary parentheses, I think here > they are needed, as this looks too close to: > > this.time = time = null agreed. same motivation I had for the other extra parenthesis above. https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... File tests/lib/logging/logging_test.dart (right): https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:9: #import('../../../lib/unittest/unittest.dart'); On 2012/06/29 03:47:45, John Messerly wrote: > do "package:" imports work yet? not until we rearrange the external repo & the SDK to have a package's directory https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:16: expect(level1 >= level1); On 2012/06/29 16:38:25, gram wrote: > On 2012/06/29 03:47:45, John Messerly wrote: > > add lessThan, lessThanOrEqual matchers? > > > > (is there any reason to prefer matchers over direct operator calls for cases > > like this?) > > Matchers will give better error messages. Unfortunately those matchers are smart and first check for == and then for <, so they wont really test that <= is implemented correctly. https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:24: expect(level2 > level1); On 2012/06/29 17:37:13, John Messerly wrote: > On 2012/06/29 16:38:25, gram wrote: > > On 2012/06/29 03:47:45, John Messerly wrote: > > > another test idea: > > > > > > test('same value but different name not equal', () { > > > var level1 = const Level('NOT_REAL1', 253); > > > var level2 = const Level('NOT_REAL_EITHER', 253); > > > expect(level2 != level1); > > > // It's like comparing NaNs! > > > expect(level2 < level1, isFalse); > > > expect(level2 <= level1, isFalse); > > > expect(level2 > level1, isFalse); > > > expect(level2 >= level1, isFalse); > > > }); > > > > I wonder - if the value is the same but the names are different, shouldn't > they > > still test as equal? > > Yeah, I thought so too. But the code for the == seemed to explicitly check for > ===. > > Whatever the behavior is, it seems like a case worth testing :) > > Oh! Another test: different instances (!==) but same name and value. changed the behavior to be == in value only. Added a test for it. https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:37: } On 2012/06/29 03:47:45, John Messerly wrote: > another test: > #import('dart:coreimpl'); > > // test they're comparable > var map = new SplayTreeMap(); > for (var level in levels) { > map[level] = level; > } > levels = map.getValues(); > for (int i = 1; i < levels.length; i++) { > expect(levels[i - 1] < levels[i]); > } added a test, but using simple list sorting instead... https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:40: test('logger naming is hierarchycal', () { On 2012/06/29 16:38:25, gram wrote: > hierarchical Done. https://chromiumcodereview.appspot.com/10693042/diff/2001/tests/lib/logging/l... tests/lib/logging/logging_test.dart:46: expect(c.parent.parent.parent.parent, isNull); On 2012/06/29 16:49:26, gram wrote: > On 2012/06/29 16:38:25, gram wrote: > > Can you explain the rationale why there are hierarchical names like this? > > I understand the intent I guess - it provides a way of controlling logging for a > subset of the logs that are grouped by function rather than by level. But it > seems quite complex, and I wonder if it would be simpler to just use a group > name (i.e. just one level of hierarchy), which could be implemented more simply. > For example, *nix logging systems do this (they have a log level and a log > 'facility', which corresponds to a functional group). doing multilevel is something I have seen a lot in other logging libraries. I've used this in the past to make an easy correlation of logs and the package/class that is creating log entries. Then it is easy to filter log entries based on classes, packages, subpackages, etc.
lgtm with a couple of comments https://chromiumcodereview.appspot.com/10693042/diff/4002/lib/logging/logging... File lib/logging/logging.dart (right): https://chromiumcodereview.appspot.com/10693042/diff/4002/lib/logging/logging... lib/logging/logging.dart:33: /** The full name of this logger, which includes also the parent's names. */ Remove 'also' https://chromiumcodereview.appspot.com/10693042/diff/4002/lib/logging/logging... lib/logging/logging.dart:35: (parent == null || parent.name == '') ? name : '${parent.fullName}.$name'; What is I have three levels A, B, C, and A is named 'A', B has name '', and C has name 'C'. Now C's fullname would be 'C', which seems wrong. It's a weird case I agree, but I think if you use hierarhical logging you are very likely going to give each level a name and the empty name should not be treated specially. https://chromiumcodereview.appspot.com/10693042/diff/4002/lib/logging/logging... lib/logging/logging.dart:55: if (_loggers.containsKey(name)) return _loggers[name]; you could have an "else" here.
https://chromiumcodereview.appspot.com/10693042/diff/4002/lib/logging/logging... File lib/logging/logging.dart (right): https://chromiumcodereview.appspot.com/10693042/diff/4002/lib/logging/logging... lib/logging/logging.dart:33: /** The full name of this logger, which includes also the parent's names. */ On 2012/06/29 19:55:21, gram wrote: > Remove 'also' Done. https://chromiumcodereview.appspot.com/10693042/diff/4002/lib/logging/logging... lib/logging/logging.dart:35: (parent == null || parent.name == '') ? name : '${parent.fullName}.$name'; On 2012/06/29 19:55:21, gram wrote: > What is I have three levels A, B, C, and A is named 'A', B has name '', and C > has name 'C'. Now C's fullname would be 'C', which seems wrong. It's a weird > case I agree, but I think if you use hierarhical logging you are very likely > going to give each level a name and the empty name should not be treated > specially. as we discussed in person - this shouldn't be a problem because the hierarchy is constructed from the names and you can't build the hierarchy by hand. There is a cornercase when names start with a '.' making (new Logger('.foo') === new Logger('foo')). I'll add a fix for that case. https://chromiumcodereview.appspot.com/10693042/diff/4002/lib/logging/logging... lib/logging/logging.dart:55: if (_loggers.containsKey(name)) return _loggers[name]; On 2012/06/29 19:55:21, gram wrote: > you could have an "else" here. true. as we briefly discussed :), left as is to make the code shorter.
On 2012/06/29 20:16:52, sigmund wrote: > https://chromiumcodereview.appspot.com/10693042/diff/4002/lib/logging/logging... > File lib/logging/logging.dart (right): > > https://chromiumcodereview.appspot.com/10693042/diff/4002/lib/logging/logging... > lib/logging/logging.dart:33: /** The full name of this logger, which includes > also the parent's names. */ > On 2012/06/29 19:55:21, gram wrote: > > Remove 'also' > > Done. > > https://chromiumcodereview.appspot.com/10693042/diff/4002/lib/logging/logging... > lib/logging/logging.dart:35: (parent == null || parent.name == '') ? name : > '${parent.fullName}.$name'; > On 2012/06/29 19:55:21, gram wrote: > > What is I have three levels A, B, C, and A is named 'A', B has name '', and C > > has name 'C'. Now C's fullname would be 'C', which seems wrong. It's a weird > > case I agree, but I think if you use hierarhical logging you are very likely > > going to give each level a name and the empty name should not be treated > > specially. > > as we discussed in person - this shouldn't be a problem because the hierarchy is > constructed from the names and you can't build the hierarchy by hand. > > There is a cornercase when names start with a '.' making (new Logger('.foo') === > new Logger('foo')). I'll add a fix for that case. > > https://chromiumcodereview.appspot.com/10693042/diff/4002/lib/logging/logging... > lib/logging/logging.dart:55: if (_loggers.containsKey(name)) return > _loggers[name]; > On 2012/06/29 19:55:21, gram wrote: > > you could have an "else" here. > > true. as we briefly discussed :), left as is to make the code shorter. LGTM! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
