API documentation cleanup by Hixie · Pull Request #108500 · flutter/flutter

Hixie · 2022-07-28T00:39:24Z

Enable asserst when running analyze_snippet_code.dart
Revamp analyze_snippet_code.dart to actually analyze all code snippets.
Add a full example for SliverOpacity.
Export src/cupertino/debug.dart which we somehow had missed.
Fix a zillion minor errors in API docs and especially API sample code.

bernaferrari · 2022-07-28T01:07:32Z

packages/flutter/lib/src/material/list_tile.dart

Two newlinews here? lol

good catch. fixed.

bernaferrari · 2022-07-28T01:21:15Z

packages/flutter/lib/src/rendering/box.dart

in ScrollPhysics you added ... without a newline.

removed the leading/trailing blank lines here

bernaferrari · 2022-07-28T01:21:32Z

packages/flutter/lib/src/rendering/box.dart

in ScrollPhysics you added ... without a newline.

a14n · 2022-07-28T07:56:18Z

dev/bots/analyze_snippet_code.dart

typo: ellipsis

a14n · 2022-07-28T08:02:33Z

dev/bots/analyze_snippet_code.dart

typo: not not

a14n · 2022-07-28T08:21:17Z

dev/bots/analyze_snippet_code.dart

Should we add num?

a14n · 2022-07-28T08:22:10Z

dev/bots/analyze_snippet_code.dart

Should we add num?

added, but i wouldn't encourage anyone to write code using num.

a14n · 2022-07-28T08:30:47Z

dev/bots/analyze_snippet_code.dart

Should we add const and extension ?

This regexp will not match top level functions or mutable variables.

Added extension, though hopefully no examples even use that.

const is intentionally not here because it's used when showing examples of Widgets, which are expressions.

Top-level functions are handled by _maybeFunctionDeclarationRegExp.

Mutable variables are hard to recognize. We never use var because of the analysis options. Generally they end up being treated as statements, which seems to work for our current examples.

a14n · 2022-07-28T08:51:58Z

dev/bots/analyze_snippet_code.dart

It looks like we can refactor this to:

Suggested change

if (line.startsWith('// // ignore_for_file: ')) {

final _Line newLine = _Line(

line: lineNumber,

indent: 3,

code: line.substring(3),

);

ignorePreambleLinesOnly.add(newLine);

preambleLines.add(newLine);

} else {

preambleLines.add(_Line(line: lineNumber, indent: 3, code: line.substring(3)));

}

final _Line newLine = _Line(line: lineNumber, indent: 3, code: line.substring(3));

if (line.startsWith('// ignore_for_file: ')) {

ignorePreambleLinesOnly.add(newLine);

}

preambleLines.add(newLine);

a14n · 2022-07-28T09:04:08Z

dev/bots/analyze_snippet_code.dart

Suggested change

if (preamble.isNotEmpty)

...preamble,

if (preamble.isNotEmpty)

const _Line.generated(), // blank line

if (preamble.isNotEmpty) ...[

...preamble,

const _Line.generated(), // blank line

],

i considered doing that but isn't that less efficient? now you're building a list just to inject it back into another list, rather than just evaluating a boolean variable twice.

a14n · 2022-07-28T09:09:47Z

dev/bots/analyze_snippet_code.dart

We can use collection-if and collection-for:

final List<_Line> codeLines = <_Line>[ if (prefix != null) _Line.generated(code: prefix), for (int i = 0; i < code.length; ++i) _Line( code: code[i], line: firstLine.line + i, indent: firstLine.indent, ), if (postfix != null) _Line.generated(code: postfix), ];

a14n · 2022-07-28T09:11:39Z

examples/api/test/widgets/sliver/sliver_opacity.1_test.dart

unnecessary wrap

Hixie · 2022-07-28T16:55:36Z

(tests are failing because I haven't landed the engine part yet)

Hixie · 2022-08-01T22:37:45Z

PR updated per comments, thanks for the reviews!

goderbauer

Thanks for cleaning this up!!!

goderbauer · 2022-08-02T00:12:27Z

packages/flutter/lib/src/material/calendar_date_picker.dart

I would expect that the alignment here will be off when these are rendered with a variable-width font on https://api.flutter.dev/?

(or does dartdoc have a special feature that I am not aware off and these will continue to render with a fixed-width font even without the ```?)

I have the same concern for many other places in this PR...

The four space indent triggers monospace font.

goderbauer · 2022-08-02T00:17:43Z

packages/flutter/lib/src/widgets/animated_list.dart

Is ... special cased somehow? Otherwise, shouldn't this be

Suggested change

/// ...

/// // ...

Now that I read the doc in analyze_snippet_code.dart, I see that it is special cased. I would argue that that isn't necessary and instead the ... should just be added as a comment. Has the added benefit that if I copy this snippet into my own code, I don't get a bunch of analyzer warnings on this line. Everywhere else (e.g. // continuing from previous example...) these are also used in comments.

yeah, seems reasonable, i'll remove this feature.

(Or more to the point, make it require // .... The feature itself is still useful, it adds an ignore.)

goderbauer · 2022-08-02T00:18:58Z

packages/flutter/lib/src/widgets/basic.dart

We should somewhere document what strings with special meaning you can use so we can point people to it in code reviews. I wonder, what would be a good place for that.

I see that they are documented in analyze_snippet_code.dart.

goderbauer · 2022-08-02T00:25:04Z

dev/bots/analyze_snippet_code.dart

Otherwise, they are... ?

technically this bullet point continues the sentence started by "Such snippets:", though admittedly by this point in the text the reader has probably forgotten this. :-)

goderbauer · 2022-08-02T00:30:58Z

dev/bots/analyze_snippet_code.dart

I wonder if we should give these control comments a common prefix. It would indicate to the reader that these comments are actually load-bearing. Also, if I want to author a new example and look at an existing one it would give me something to search for to understand what other control comments are available.

On the other hand, it my hinder the reading flow of these examples...

Just a thought, not feeling super-strongly about this either way.

yeah i feel super constrained here because the most important result is that the examples flow well for the developers.
on the other hand, these load-bearing comments are so magical, ugh.

I've added a message to the snippet analyzer's output that just points right at the docs, FWIW. Dunno how much it'll help.

Hixie · 2022-08-03T23:10:36Z

Landed fixes for the dev/bots/test tests.

See flutter/flutter#108500

Hixie · 2022-08-03T23:12:46Z

This depends on flutter/engine#35146 rolling into the framework.

goderbauer

RSLGTM when checks are happy.

Hixie · 2022-08-08T18:26:08Z

Now blocked on flutter/engine#35235

auto-submit · 2022-08-08T22:18:06Z

The status or check suite Linux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label.

* Enable asserst when running analyze_snippet_code.dart * Revamp analyze_snippet_code.dart to actually analyze _all_ code snippets. * Add a full example for SliverOpacity. * Export src/cupertino/debug.dart which we somehow had missed. * Fix a zillion minor errors in API docs and especially API sample code.

bernaferrari reviewed Jul 28, 2022

View reviewed changes

a14n reviewed Jul 28, 2022

View reviewed changes

Hixie mentioned this pull request Jul 29, 2022

Minor fix compendium #107874

Merged

Hixie force-pushed the snippets branch from 276a8d2 to 8b7d5f1 Compare August 1, 2022 22:37

goderbauer reviewed Aug 2, 2022

View reviewed changes

Hixie force-pushed the snippets branch 3 times, most recently from 4b573c0 to 1e667aa Compare August 3, 2022 23:09

Hixie added a commit to Hixie/sky_engine that referenced this pull request Aug 3, 2022

We decided not to support bare ellipsis in the API docs.

5208b68

See flutter/flutter#108500

Hixie mentioned this pull request Aug 3, 2022

We decided not to support bare ellipsis in the API docs. flutter/engine#35146

Merged

goderbauer approved these changes Aug 3, 2022

View reviewed changes

Hixie force-pushed the snippets branch from 1e667aa to 91010ad Compare August 8, 2022 18:18

Hixie force-pushed the snippets branch 2 times, most recently from d4f1fc3 to 4fc72cc Compare August 8, 2022 22:09

Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 8, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 8, 2022

Hixie force-pushed the snippets branch 5 times, most recently from 0c2b01d to 9cf960d Compare August 10, 2022 19:31

Hixie force-pushed the snippets branch from 9cf960d to 8aec95d Compare August 10, 2022 19:49

Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 10, 2022

auto-submit bot merged commit 7ded3d9 into flutter:master Aug 10, 2022

Hixie deleted the snippets branch August 10, 2022 22:23

This was referenced Aug 11, 2022

Roll Flutter from 3d2de8b1d8f6 to c91b481263da (23 revisions) flutter/packages#2442

Merged

Roll Flutter from ac2f6ed23784 to c91b481263da (20 revisions) flutter/plugins#6225

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 11, 2022

7ded3d9 API documentation cleanup (flutter/flutter#108500)

d9e83c3

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 11, 2022

7ded3d9 API documentation cleanup (flutter/flutter#108500)

cd76608

engine-flutter-autoroll mentioned this pull request Aug 11, 2022

Roll Flutter from ac2f6ed23784 to 9ad052a57e0f (21 revisions) flutter/plugins#6226

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 11, 2022

7ded3d9 API documentation cleanup (flutter/flutter#108500)

131d545

-              if (line.startsWith('// // ignore_for_file: ')) {
-                final _Line newLine = _Line(
-                  line: lineNumber,
-                  indent: 3,
-                  code: line.substring(3),
-                );
-                ignorePreambleLinesOnly.add(newLine);
-                preambleLines.add(newLine);
-              } else {
-                preambleLines.add(_Line(line: lineNumber, indent: 3, code: line.substring(3)));
-              }
+              final _Line newLine = _Line(line: lineNumber, indent: 3, code: line.substring(3));
+              if (line.startsWith('// ignore_for_file: ')) {
+                ignorePreambleLinesOnly.add(newLine);
+              }
+              preambleLines.add(newLine);

-        if (preamble.isNotEmpty)
-          ...preamble,
-        if (preamble.isNotEmpty)
-          const _Line.generated(), // blank line
+        if (preamble.isNotEmpty) ...[
+          ...preamble,
+          const _Line.generated(), // blank line
+        ],

Conversation

Hixie commented Jul 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jul 28, 2022

Uh oh!

Hixie commented Aug 1, 2022

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!