Responding Comments on a Dart Change Log

‹prev | My Chain | next›

While I have been futzing about with Dart powered HTTP servers, it turns out that the patch that I had submitted for a dartdoc bug has been receiving plenty of feedback. I do not know how I managed to submit a change log without being subscribed to activity notifications, but manage it, I did.

I would have gone on ignoring it indefinitely had Andrei Mouravski not pinged me directly wondering if there was anything he could do to help me respond to his comments. How cool is it that the Dart team is that on top of things?

Anyhow, I need to make a few changes and resubmit the patch. The actual code patch remains a two character fix (adding a newline character). The problem noted in the change log was with the test. As I worried, the paths in the test that I submitted were not correct.

Also in the patch comments, I was asked to split a line of code to make it less than 80 characters wide. I get nervous when code is more than 60 characters wide, so I am more than happy to accommodate.

The last thing that I was asked to try was to add normal code indentation to my fake source location object. So, instead of:

class FakeSourceLocation implements SourceLocation {
  // ...
  String get sourceText => """
/// Testing
///     var testing = 'this is source code';
get foo => 'bar';
""";
}

I try this instead:

class FakeSourceLocation implements SourceLocation {
  // ...
  String get sourceText => """
  /// Testing
  ///     var testing = 'this is source code';
  get foo => 'bar';
""";
}

Only when I make that change, my passing test now fails with:

➜  dart-repo  ./dart/tools/testing/bin/linux/dart ./dart/sdk/lib/_internal/dartdoc/test/comment_map_test.dart
unittest-suite-wait-for-done
FAIL: triple slashed comments retain newlines
  Expected: 'Testing\n    var testing = 'this is source code';'
       but: was <null>.
...  
0 PASSED, 1 FAILED, 0 ERRORS

So it seems as though that proper indentation is not going to work. It seemed a reasonable thing to try. By making the code a closer approximation of real-world dartdoc, this test would be more apt to catch regressions.

Now the question becomes, is this failing because there is still a bug or because source location objects already strip out leading whitespace? Well, a bit of digging reveals a third explanation—I need to adjust one more thing in the test. In addition to the dartdoc, my fake source location also needs to provide a count of the number of characters until the actual code. It had been 57. With three lines newly indented by 2 spaces, I need to adjust that number by 6 to 63:

class FakeSourceLocation implements SourceLocation {
  // ...
  int get offset => 63;
  String get sourceText => """
  /// Testing
  ///     var testing = 'this is source code';
  get foo => 'bar';
""";
}

With that, I am back to a passing test:

➜  dart git:(code-samples-for-triple-slash-dartdoc) ✗ ./tools/testing/bin/linux/dart ./sdk/lib/_internal/dartdoc/test/comment_map_test.dart
unittest-suite-wait-for-done
PASS: triple slashed comments retain newlines

All 1 tests passed.
unittest-suite-success

After committing those changes to my local git repository, I now need to upload back to the code review. The next step is git cl upload. I am a little worried that this will not get associated with the current change log, but fingers crossed:

➜  dart git:(code-samples-for-triple-slash-dartdoc) ✗ git cl upload
Using 50% similarity for rename/copy detection. Override with --similarity.
Loaded authentication cookies from /home/chris/.codereview_upload_cookies
Running presubmit upload checks ...

Presubmit checks passed.
 .../dartdoc/lib/src/dartdoc/comment_map.dart       |    2 +-
 .../_internal/dartdoc/test/comment_map_test.dart   |   38 ++++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)
This branch is associated with issue 11358134. Adding patch to that issue.
Upload server: https://codereview.chromium.org/ (change with -s/--server)
Loaded authentication cookies from /home/chris/.codereview_upload_cookies
Title describing this patch set: Address code review comments
Issue updated. URL: https://codereview.chromium.org/11358134

Yay! It did remember the original change log. After a quick bit of digging, I find that this information goes in .git/config:

# ...
[svn-remote "svn"]
        url = http://dart.googlecode.com/svn/branches/bleeding_edge/dart
        fetch = :refs/remotes/git-svn
[rietveld]
        server = https://codereview.chromium.org/
        cc = reviews@dartlang.org
        viewvc-url = https://code.google.com/p/dart/source/detail?r=
[branch "code-samples-for-triple-slash-dartdoc"]
        rietveldissue = 11358134
        rietveldserver = https://codereview.chromium.org/
        rietveldpatchset = 6001

I will probably never need to know how it stores that information, but I am still glad to know it. With that, I go back to the ticket, and click the "Publish+Mail Comments" link to notify my reviewers that it is ready for another look:

And now I wait. But this time, I am definitely subscribed to notifications, so I won't miss any more action.

Again, huge thanks to Andrei Mouravski for directly following up with me after I seemingly ignored his comments completely for a week. I really appreciate it!


Day #571