support empty line comments

[devoncarew]

• support empty line comments

This is to allow us to do things like:

// Copyright foo
// bar
// baz.

// Here's an additional file comment, but separated from the copyright header.

---

• I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

• See our contributor guide for general expectations for PRs.
• Larger or significant changes should be discussed in an issue before creating a PR.
• Contributions to our repos should follow the Dart style guide and use dart format.
• Most changes should add an entry to the changelog and may need to rev the pubspec package version.
• Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

[github-actions]

Package publishing

Package	Version	Status	Publish tag (post-merge)
package:code_builder	4.8.1	ready to publish	v4.8.1

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

[srawlins]

It seems like there should still be a way to get this output. Not sure how though... comments could be a list of nullable strings, and then null means something different from a blank string. Or a string that is only whitespace vs blank? I can't think of anything great...

[devoncarew]

I don't know that it's an important use case to support // lines? But the easiest and most clear way would likely be by supporting null strings.

An alternative approach would be to support line wrapping. So, multiple lines would mean multiple groups of comments, with each line automatically wrapped at ~80 cols.

I may implement the 1st solution as that seems like the smallest API diff.

[srawlins]

Well, I don't have any examples, but I can imagine that it's more common to write a multi-paragraph comment (with blank // lines) than adjacent comments. Like if you have two paragraphs, or an example, like a code block separated from test.

[devoncarew]

This functionality is just affecting leading library line comments; mostly this is used for the ~3 line copyright headers for files.

The use case I have here is that I also want a // Generated by ... message at the top of the file. Currently this is emitted as part of the file copyright header, which is really not intended or desired. So, I want to go from:

// Copyright (c) 2023, 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.
//
// Generated from Web IDL definitions.

to:

// Copyright (c) 2023, 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.

// Generated from Web IDL definitions.

(see https://github.com/dart-lang/web/blob/main/lib/src/dom/accelerometer.dart)

[devoncarew]

ptal - updated to support both empty line comments as well as having groups of line comments

[srawlins]

I'm not a huge fan of the nullable String API, where null is sort of a "magic value." But I do think it is extremely pragmatic, functional, easy to understand...

I think I'd like another opinion like @natebosch before landing. If we go a nullable String API then it would be a pain to bring it back to a non-nullable String API. I'd almost prefer an API where "a comment" is a List<String> and a declaration, like a class, can have "plural comments", so a List<Comment>. But that might not be pragmatic, making everyone write so many nested structures just to add a little one-line comment somewhere...

[devoncarew]

I'm not a huge fan of the nullable String API, where null is sort of a "magic value." But I do think it is extremely pragmatic, functional, easy to understand...

Agree w/ the nullability aspect; I think it doesn't improve the API. I've reverted back to a simplier form of this PR; happy to wait for an opinion from Nate.

For context, this PR is to improve the output of the files we generate for package:web.

[natebosch]

I'd almost prefer an API where "a comment" is a List<String> and a declaration, like a class, can have "plural comments", so a List<Comment>. But that might not be pragmatic, making everyone write so many nested structures just to add a little one-line comment somewhere...

It's currently a bug to have a newline in a comment. We could allow this flexibility by splitting each String by newlines.

diff --git a/lib/src/emitter.dart b/lib/src/emitter.dart
index 591b992..fe4a898 100644
--- a/lib/src/emitter.dart
+++ b/lib/src/emitter.dart
@@ -2,6 +2,8 @@
 // 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.
 
+import 'dart:convert';
+
 import 'allocator.dart';
 import 'base.dart';
 import 'specs/class.dart';
@@ -479,8 +481,16 @@ class DartEmitter extends Object
     output ??= StringBuffer();
 
     if (spec.comments.isNotEmpty) {
-      spec.comments.map((line) => '// $line').forEach(output.writeln);
-      output.writeln();
+      for (final comment in spec.comments) {
+        for (final line in const LineSplitter().convert(comment)) {
+          if (line.isEmpty) {
+            output.writeln('//');
+          } else {
+            output.writeln('// $line');
+          }
+        }
+        output.writeln();
+      }
     }
 
     if (spec.ignoreForFile.isNotEmpty) {
diff --git a/test/specs/library_test.dart b/test/specs/library_test.dart
index 3c5502e..a525ae2 100644
--- a/test/specs/library_test.dart
+++ b/test/specs/library_test.dart
@@ -35,9 +35,7 @@ void main() {
         Library(
           (b) => b
             ..comments.addAll([
-              'Generated by foo!',
-              '',
-              'Avoid editing by hand.',
+              'Generated by foo!\n\nAvoid editing by hand.',
             ])
             ..body.add(
               Class((b) => b..name = 'Foo'),

I think this change, either my diff or as is in the PR, breaks the meaning of some usage in the same way. The line splitting gives users an escape hatch to get back to the old output if that is what they prefer.

[devoncarew]

Thinking about the comments above:

• I don't love changing the meaning of the current comments field (what this PR proposes and in Nate's idea above)
• I think a green field solution would use something like the List<Comment> idea from above, but I don't think the new api / deprecated api dance for a new comments field is worth it for this feature
• the use case I have is wanting both a file copyright section and a 'generated by' line for a generated file (and for that 2nd comment to be separate from the copyright one)

I have a separate PR now - #441 - which adds a new field for that specific use case - a generatedByComment field. This is not dissimilar to the existing Library.ignoreForFile field.