Tuesday, August 27, 2013

Deprecating Method Parameters in Dart


I wound up investigating the @proxy code annotation in Dart last night. In a testament to how easily distracted I am, I was looking in the documentation for the @deprecated annotation. In fact, I am about three distractions away from my original intent, which is updating Dart for Hipsters. But I am making progress. Really!

I was investigating the @deprecated annotation as a potential means to deprecate some callbacks in dart dirty. Who needs callbacks when there are Futures around? Well, no one, really, which is why I am deprecating them in methods like close():
class Dirty implements HashMap<String, Object> {
  // ...
  void close([cb()=_default_cb]) {
    _io.close().
      then((_) => cb());
  }
}
It is just as easy, if not easier, to return a Future from that method. Programmers that need to perform an action once file I/O is closed can wait for the Future to complete, then() perform whatever action they desire.

Ultimately, this will look like:
class Dirty implements HashMap<String, Object> {
  // ...
  Future<File> close() {
    return _io.close()
      .then((_) => _db);
  }
}
The implementation is just as concise as the old way with the added benefit about being specific when documenting that the Future will complete with a File instance. In addition to being “Dartier,” this will be cleaner to use:
      db.close().
        then((f) {
          // Do something with the DB file here...
        });
It is really a no-brainer to switch to Futures. The only question is how to deprecate a single argument.

If I mark the close() method as deprecated:
  // ...
  @deprecate // the callback argument is not longer supported
  Future<File> close([cb()=_default_cb]) {
    return _io.close()
      .then((_) => _db)
        ..then((_) => cb());
  }
  // ...
Then I get a nice little strike-through in Dart Editor:



But that is of no use in this case. I do not want to deprecate the entire method, just the old callback.

Also, if I am relying on dartanalyzer to catch potential static typing and other issues, then I am out of luck with deprecations. As far as dartanalyzer is concerned, there is nothing wrong with calling deprecated methods:
➜  dart-dirty git:(master) ✗ dartanalyzer --hints test/dirty_test.dart
Analyzing test/dirty_test.dart...
No issues found.
Still, I want to be a good package maintainer, so what are my options?

Realistically, I doubt that many people are using dart-dirty. Even if they are, few would much care for the old callback approach. So I could just change it. I may opt for that approach, but I would like to see what is possible.

If a callback is called, I would like to output a warning of some kind—preferably a scary looking one—indicating that the callback will be going away very soon. But, as a good package maintainer, I still want to call the callback. Most importantly, the return value of close() should be a Future containing a File object.

I opt to modify close() so that it cascades after the Future containing the File object:
  Future close([cb()]) {
    return _io.close()
      .then((_) => _db)
        ..then((_) => _deprecateCallback(cb));
  }
  // ...
The double dot cascade will return its receiver—the result of the previous then() call, which is the Future<File> that I want. The cascaded method will still be invoked and that _deprecateCallback() private method can do what I need it to—print out the obnoxious warning message and call the callback:
 // ...
  _deprecateCallback(cb()) {
    if (cb == null) return;
    _deprecate('callbacks no longer supported');
    cb();
  }
  // ...
To make the message look as scary as possible, it ought to look like a stack trace. It is not possible to directly create stack traces in Dart, but it is possible to throw Errors, which have a stack trace property:
  // ...
  _deprecate(String message) {
      try {
        throw new ArgumentError('DEPRECATED: $message');
      }
      on Error catch (e) {
        print('''
DEPRECATED ${e}:
${e.stackTrace.toString().split("\n").take(3).join("\n")}
...
''');
      }
  }
  // ...
I only take the first 3 lines in the stack trace because most of it is many layers of Future guts. In the end this does reasonably well:
DEPRECATED Illegal argument(s): DEPRECATED: callbacks no longer supported:
#0      Dirty.close.<anonymous closure> (package:dirty/dirty.dart:92:15)
#1      _ThenFuture._zonedSendValue (dart:async/future_impl.dart:371:24)
#2      _TransformFuture._sendValue.<anonymous closure> (dart:async/future_impl.dart:348:48)
...

DEPRECATED Illegal argument(s): DEPRECATED: callbacks no longer supported:
#0      Dirty.close.<anonymous closure> (package:dirty/dirty.dart:92:15)
#1      _ThenFuture._zonedSendValue (dart:async/future_impl.dart:371:24)
#2      _TransformFuture._sendValue.<anonymous closure> (dart:async/future_impl.dart:348:48)
...
It would have been nice if the context of the original call was included in the stack trace, but alas, futures. Still, this should be sufficient to help library users to understand what needs to be fixed.

After converting any tests that used to rely on the old callback, I run my test suite to verify that no functionality is broken and, more importantly, that dartanalyzer is satisfied that I am not up to any shenanigans:
PASS: new DBs creates a new DB
PASS: writing can write a record to the DB
PASS: reading can read a record from the DB
PASS: reading can read a record from the DB stored on the filesystem
PASS: removing can remove a record from the DB
PASS: removing can remove keys from the DB
PASS: removing can remove a record from the filesystem store
PASS: removing removes from the list of keys in the filesystem store
PASS: removing can delete the DB entirely from the filesystem

All 9 tests passed.
unittest-suite-success

dartanalyzer lib/dirty.dart
Analyzing test/dirty_test.dart...
No issues found.
Analyzing lib/dirty.dart...
No issues found.
That may be a touch of overkill. Then again, I do want to be a good package maintainer, so hopefully this exercise at least puts me in the right frame of mind. But the most important takeaway is that I will never again use callbacks in Dart code.


Day #856

No comments:

Post a Comment