first version of a logging library.

lib/logging/logging.dart

Index: lib/logging/logging.dart
diff --git a/lib/logging/logging.dart b/lib/logging/logging.dart
new file mode 100644
index 0000000000000000000000000000000000000000..d1f6abdeb8876118aeac9f11862cb671039cdd8d
--- /dev/null
+++ b/lib/logging/logging.dart
@@ -0,0 +1,287 @@
+// Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+/**
+ * Provides APIs for debugging and error logging. This library introduces
+ * abstractions similar to those used in other languages, such as the Closure JS
+ * library and java.util.logging.

[Jennifer Messerly, 2012/06/29 03:47:45]

did you mean: "such as the Closure JS Logger"

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Done :)

+ */
+#library('logging');
+
+/** A handler that process log entries in of a [Logger]. */

[gram, 2012/06/29 16:38:25]

Grammar? I can't parse this sentence.

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Done (also moved further down...)

+typedef void LoggerHandler(LogRecord);
+
+/** Default level when none is set on a logger. */
+Level _rootLevel = Level.INFO;

[gram, 2012/06/29 16:38:25]

Shouldn't this be called _defaultLevel then?

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

rephrased doc

+
+
+/**
+ * Use a [Logger] to log debug messages. [Logger]s are named using a
+ * hierarchical dot-separated name convention.
+ */
+class Logger {
+  /** Simple name of this logger. */
+  String name;

[Jennifer Messerly, 2012/06/29 03:47:45]

should this be final? It seems like changing the name after creating the Logger
will mess up the _loggers Map

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Good point. Done.

+
+  /* The full name of this logger, which includes also the parent's names. */

[Jennifer Messerly, 2012/06/29 03:47:45]

/**

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Done.

+  String get fullName() => 
+      (parent == null || parent.name == '') ? name : '${parent.fullName}.$name';

[Jennifer Messerly, 2012/06/29 03:47:45]

If I'm understanding this correctly: every logger gets a parent, until we reach
empty-string, and that one has null parent?

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Correct.

+
+  /** Parent of this logger in the hierarchy of loggers. */
+  Logger parent;

[Jennifer Messerly, 2012/06/29 03:47:45]

should this be final too? as well as the other fields. (except _level)

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Done. _handlers and children are not final so we avoid creating empty maps/lists
when they are not used.

+
+  /** Logging [Level] used for entries generated on this logger. */
+  Level _level;
+
+  /** Children in the hierarchy of loggers, indexed by their simple names. */
+  Map<String, Logger> children;
+
+  /** Handlers used to process log entries in this logger. */
+  List<LoggerHandler> _handlers;
+
+
+  /** 
+   * Singleton constructor. Calling [:new Logger(name) :] will return the same

[Jennifer Messerly, 2012/06/29 03:47:45]

nit, use backticks: `new Logger(name)`
http://daringfireball.net/projects/markdown/syntax/#code

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

done, but [: :] is the dart doc way :)

+   * actual instance whenever it is called with the same string name.
+   */
+  factory Logger(String name) {
+    if (_loggers == null) _loggers = new Map<String, Logger>();

[Jennifer Messerly, 2012/06/29 03:47:45]

you could use "<Logger>{}" instead of "new Map<String, Logger>()". Personal
preference though. Either works for me.

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Done.

+    if (_loggers.containsKey(name)) return _loggers[name];
+
+    // Split hierarchical names (separated with '.').
+    int dot = name.lastIndexOf('.');
+    String parentName;

[Jennifer Messerly, 2012/06/29 03:47:45]

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);

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

I like it, very nice suggestion. Done.

+    String thisName;
+    if (dot == -1) {
+      parentName = name == '' ? null : '';
+      thisName = name;
+    } else {
+      parentName = name.substring(0, dot);
+      thisName = name.substring(dot + 1);
+    }
+    final res = new Logger._internal(thisName,
+        parentName == null ? null : new Logger(parentName));
+    _loggers[name] = res;
+    return res;
+  }
+
+  Logger._internal(this.name, this.parent) :
+    children = new Map<String, Logger>() {

[Jennifer Messerly, 2012/06/29 03:47:45]

nit: indent 4 from Logger then ": children = " ...

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Nice - I had forgotten, the style guide has the style that I actually liked :)

+    if (parent != null) parent.children[name] = this;
+  }
+
+  /** 
+   * Effective level considering the levels established in this logger's parents
+   * (when [enableHierarchyLogging] is true).
+   */
+  Level get level() {
+    if (enableHierarchyLogging) {
+      if (_level != null) return _level;
+      if (parent != null) return parent.level;
+    }
+    return _rootLevel;
+  }
+
+  /** Override the level for this particular [Logger] and its children. */
+  Level set level(value) {
+    if (enableHierarchyLogging && parent != null) {
+      _level = value;
+    } else {
+      if (parent != null) throw new Exception(

[Jennifer Messerly, 2012/06/29 03:47:45]

UnsupportedOperationException

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Done

+          'Cannot set level on a non-root logger when hierarchycal logging '

[Jennifer Messerly, 2012/06/29 03:47:45]

typo: should be "hierarchical"

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Done, thanks!

+          'is not enabled');

[Jennifer Messerly, 2012/06/29 03:47:45]

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 :)

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Done.

+      _rootLevel = value;
+    }
+  }
+
+  /** Adds a handler to listen whenever a log record is added to this logger. */
+  void addHandler(LoggerHandler handler) {

[Jennifer Messerly, 2012/06/29 03:47:45]

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"

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Done - added the pattern, yippie! Used 'on.record' rather than 'on.message'
(since what you get are log records)

+    if (enableHierarchyLogging || parent == null) {
+      if (_handlers == null) {
+        _handlers = new List<LoggerHandler>();
+      }
+      _handlers.add(handler);
+    } else {
+      root.addHandler(handler);
+    }
+  }
+
+  /** Remove an previously added handler. */

[gram, 2012/06/29 16:38:25]

Remove a ...

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

thx. done

+  void removeHandler(LoggerHandler handler) {
+    if (enableHierarchyLogging || parent == null) {
+      if (_handlers == null) return;
+      int index = _handlers.indexOf(handler);
+      if (index != -1) _handlers.removeRange(index, 1);
+    } else {
+      root.removeHandler(handler);
+    }
+  }
+
+  /** Removes all handlers previously added to this logger. */
+  void clearHandlers() {
+    if (enableHierarchyLogging || parent == null) {

[gram, 2012/06/29 16:38:25]

Can this be called 'hierarchicalLoggingEnabled' instead? It's a flag so this
should be a verb.

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Done.

+      _handlers = null;
+    } else {
+      root.removeHandler(handler);
+    }
+  }
+
+  /** Whether a message for [value]'s level is loggable in this logger. */
+  bool isLoggable(Level value) => (value >= level);

[Jennifer Messerly, 2012/06/29 03:47:45]

nit: redundant parens.

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

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.

+
+  // TODO(sigmund): add support for logging exceptions.
+  void log(Level logLevel, String message) {

[Jennifer Messerly, 2012/06/29 03:47:45]

This could use a doc comment too, like the other excellent ones in this file :)

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Done. 

+    if (isLoggable(logLevel)) {
+      var record = new LogRecord(logLevel, message, fullName);
+      if (enableHierarchyLogging) {
+        var target = this;
+        while (target != null) {
+          target._publish(record);
+          target = target.parent;
+        }
+      } else {
+        root._publish(record);
+      }
+    }
+  }
+
+  /** Log message at level [Level.FINEST]. */
+  void finest(Strnig message) => log(Level.FINEST, message);

[gram, 2012/06/29 16:38:25]

Typo, here and elsewhere: Strnig

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

wow! 

running tests in checked mode -- checked!

+
+  /** Log message at level [Level.FINER]. */
+  void finer(Strnig message) => log(Level.FINER, message);
+
+  /** Log message at level [Level.FINE]. */
+  void fine(Strnig message) => log(Level.FINE, message);
+
+  /** Log message at level [Level.CONFIG]. */
+  void config(Strnig message) => log(Level.CONFIG, message);
+
+  /** Log message at level [Level.INFO]. */
+  void info(Strnig message) => log(Level.INFO, message);
+
+  /** Log message at level [Level.WARNING]. */
+  void warning(Strnig message) => log(Level.WARNING, message);
+
+  /** Log message at level [Level.SEVERE]. */
+  void severe(Strnig message) => log(Level.SEVERE, message);
+
+  /** Log message at level [Level.SHOUT]. */
+  void shout(Strnig message) => log(Level.SHOUT, message);
+
+  void _publish(LogRecord record) {
+    if (_handlers != null) {
+      _handlers.forEach((h) => h(record));
+    }
+  }
+
+  /** Top-level root [Logger]. */
+  static get root() => new Logger('');
+
+  /**
+   * Whether to allow fine-grain logging and configuration of loggers in a
+   * hierarchy. When false, all logging is merged in the root logger.
+   */
+  static bool enableHierarchyLogging = false;

[Jennifer Messerly, 2012/06/29 03:47:45]

does this need to be configurable?

might be worth renaming this "enableHierarchy", since the logging part is
implied by the "Logger."

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

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

+
+  /** All [Logger]s in the system. */
+  static Map<String, Logger> _loggers;
+}
+
+
+/**
+ * [Level]s to control logging output. Logging can be enabled to include all
+ * levels above certain [Level]. [Level]s are ordered using an integer
+ * value [Level.value]. The predefined [Level] constants below are sorted as
+ * follows (in descending order): [Level.SHOUT], [Level.SEVERE],
+ * [Level.WARNING], [Level.INFO], [Level.CONFIG], [Level.FINE], [Level.FINER],
+ * [Level.FINEST], and [Level.ALL].
+ *
+ * We recommend using one of the predefined logging levels. If you define your
+ * own level, make sure you use a value between those used in [Level.ALL] and
+ * [Level.OFF].
+ */
+class Level {

[Jennifer Messerly, 2012/06/29 03:47:45]

implements Comparable, Hashable

and add methods:
  int compareTo(Level other) => value - other.value;
  hashCode() => value;

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Done.

+  final String name;
+
+  /**
+   * Unique value for this level. Used to order levels, so filtering can exclude
+   * messages whose level is under certain value.
+   */
+  final int value;

[Jennifer Messerly, 2012/06/29 03:47:45]

TODO: this should be "const" when that modifier is supported.
Likewise for the "name" field

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Done

+
+  const Level(this.name, this.value);
+
+  /** Special key to turn on logging for all levels ([value] = 0). */
+  static final Level ALL = const Level('ALL', 0);
+
+  /** Special key to turn off all logging ([value] = 2000). */
+  static final Level OFF = const Level('OFF', 2000);
+
+  /** Key for highly detailed tracing ([value] = 300). */
+  static final Level FINEST = const Level('FINEST', 300);
+
+  /** Key for fairly detailed tracing ([value] = 400). */
+  static final Level FINER = const Level('FINER', 400);
+
+  /** Key for tracing information ([value] = 500). */
+  static final Level FINE = const Level('FINE', 500);
+
+  /** Key for static configuration messages ([value] = 700). */
+  static final Level CONFIG = const Level('CONFIG', 700);
+
+  /** Key for informational messages ([value] = 800). */
+  static final Level INFO = const Level('INFO', 800);
+
+  /** Key for potential problems ([value] = 900). */
+  static final Level WARNING = const Level('WARNING', 900);
+
+  /** Key for serious failures ([value] = 1000). */
+  static final Level SEVERE = const Level('SEVERE', 1000);
+
+  /** Key for extra debugging loudness ([value] = 1200). */
+  static final Level SHOUT = const Level('SHOUT', 1200);
+
+  bool operator ==(Level other) => this === other;

[Jennifer Messerly, 2012/06/29 03:47:45]

do you need to define this?

[Jennifer Messerly, 2012/06/29 17:37:13]

as gram mentioned later, maybe this should be "value == other.value". That seems
more consistent with the other comparison methods.

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

gone

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

agreed - done.

+  bool operator <(Level other) => value < other.value;
+  bool operator <=(Level other) => value <= other.value;
+  bool operator >(Level other) => value > other.value;
+  bool operator >=(Level other) => value >= other.value;
+
+  String toString() => name;
+}
+
+
+/**
+ * A log entry representation used to propagate information from [Logger] to
+ * individual [Handler]s.
+ */
+class LogRecord {
+  final Level level;
+  final String message;
+
+  /** Logger where this record is stored. */
+  final String loggerName;
+
+  /** Time when this record was created. */
+  final Date time;
+
+  /** Unique sequence number greater than all log records created before it. */
+  final int sequenceNumber;
+
+  static int _nextNumber = 0;
+
+  /** Associated exception (if any) when recording errors messages. */
+  Exception exception;
+
+  /** Associated exception message (if any) when recording errors messages. */
+  String exceptionText;
+
+  LogRecord(
+      this.level, this.message, this.loggerName, 
+      [time, sequenceNumber, this.exception, this.exceptionText]) :

[gram, 2012/06/29 16:38:25]

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.

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

Removed from now. Just made this as similar as possible to the JS Closure
implementation.

+    this.time = time == null ? new Date.now() : time,

[Jennifer Messerly, 2012/06/29 03:47:45]

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 =

[gram, 2012/06/29 16:38:25]

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

[Siggi Cherem (dart-lang), 2012/06/29 18:34:48]

agreed. same motivation I had for the other extra parenthesis above. 

+    this.sequenceNumber =
+        sequenceNumber == null ? LogRecord._nextNumber++ : sequenceNumber;
+}