Skip to content
This repository has been archived by the owner. It is now read-only.

BuildLogger does not correctly implement TransformLogger #38

Closed
jmesserly opened this Issue Mar 24, 2016 · 4 comments

Comments

1 participant
@jmesserly
Copy link
Contributor

commented Mar 24, 2016

TransformLogger.warning, for example, expects the first argument to be a String.

BuildLogger by contrast, expects it to be a Message:

class BuildLogger implements TransformLogger {
  // ...

  /// Records a warning message. If [msg] is a [Message] it is recorded
  /// directly, otherwise it is first converted to a [String].
  void warning(msg, {AssetId asset, SourceSpan span}) {
    msg = msg is Message ? msg : new Message.unknown('$msg');
    _transform.logger.warning(_snippet(msg), asset: asset, span: span);
    _logs.add(new BuildLogEntry(msg, span, LogLevel.WARNING.name));
  }

In strong mode, it infers "msg" as String.

It is possible to make this sound by typing "msg" as "Object msg". That's probably the short term fix for strong mode.

In the longer term, this seems like a non-ideal API design.

@jmesserly

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2016

I hit this in the observe package, it tries to pass objects to the "msg" field, but it gets an error because they are not strings.

@jmesserly

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2016

Note that the assignment msg = msg is Message ? msg : new Message.unknown('$msg'); is also an error in strong mode (because msg is a String)

@jmesserly

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2016

@jmesserly

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2016

fixed in cde6c50

@jmesserly jmesserly closed this Mar 25, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.