Closed devoncarew closed 6 months ago
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.
ptal - updated to support both empty line comments as well as having groups of line comments
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...
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.
I'd almost prefer an API where "a comment" is a
List<String>
and a declaration, like a class, can have "plural comments", so aList<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.
Thinking about the comments above:
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 featureI have a separate PR now - https://github.com/dart-lang/code_builder/pull/441 - which adds a new field for that specific use case - a generatedByComment
field. This is not dissimilar to the existing Library.ignoreForFile
field.
This is to allow us to do things like:
Contribution guidelines:
- See our [contributor guide](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md) 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](https://dart.dev/guides/language/effective-dart) and use `dart format`. - Most changes should add an entry to the changelog and may need to [rev the pubspec package version](https://github.com/dart-lang/sdk/wiki/External-Package-Maintenance#making-a-change). - Changes to packages require [corresponding tests](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md#Testing). Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.