Migrate dartdoc_options.dart #2749

jcollins-g · 2021-08-18T23:17:37Z

This one was tricksy because it involves argument parsing and yaml parsing, two areas that are null minefields.

Significant nullability choices:

.parent is mostly non-nullable except for a specially defined root node that will throw if you try to access its parent. Makes things easier to read, but the way that trees of options are constructed doesn't quite sit right with me now -- those late declarations hang around too long before being written to. Added a TODO around that as I don't have a good alternative just yet.
valueAt is nullable. It's just too hard to try to restrict that given how package:args works and yaml giving us nulls. If we happen to know some values aren't nullable, we can wrap them in the future.
operator [] is non-nullable. I think this is the right choice as child nodes in the option tree are defined by the programmer, and we should never be trying to look up an undefined option. Changing this found some bugs, too.

Most everything else follows naturally from these choices and from #2745.

* more specific imports in options * do not allow autoinitialization * dartfmt * parameterize the valueWithContext type * Use alternative constructors instead of externalizing, didn't realize the autodetect was so widely used * more subclasses

jcollins-g · 2021-08-18T23:26:11Z

Also, the tests are not migrated; after running into trouble and reading this, I've decided trying to work around the problem of entry points triggering strong nullability checks by default in tests is too much to be worth it. So we'll just settle for a Test Skitching Development style.

srawlins

Love it

srawlins · 2021-08-19T16:40:16Z

lib/src/dartdoc_options.dart

      var categoryMap = entry.value;
      if (categoryMap is Map) {
-        displayName = categoryMap['displayName']?.toString();
-        documentationMarkdown = categoryMap['markdown']?.toString();
+        var displayName = categoryMap['displayName']?.toString();


haha easy win

Yeah, my early Dart code was a little cringey, I'm happy to give it a touch up as part of this migration :-)

srawlins · 2021-08-19T16:40:32Z

lib/src/dartdoc_options.dart

-  ToolRunner _runner;
-
-  ToolRunner get runner => _runner ??= ToolRunner(this);
+  late ToolRunner runner = ToolRunner(this);


srawlins · 2021-08-19T16:41:39Z

lib/src/dartdoc_options.dart

-    pathContext = p.Context(current: canonicalDirectoryPath);
-  }
+  _OptionValueWithContext(this.value, String path, {this.definingFile})
+      : canonicalDirectoryPath = p.canonicalize(path),


+1 nice refactor

srawlins · 2021-08-19T16:43:51Z

lib/src/dartdoc_options.dart

    var value = valueWithContext.value;
    if (value is String) {
      resolvedPaths = [valueWithContext.resolvedValue as String];
    } else if (value is List<String>) {
      resolvedPaths = valueWithContext.resolvedValue as List<String>;
    } else if (value is Map<String, String>) {
-      resolvedPaths = (valueWithContext.resolvedValue as Map).values.toList();
+      resolvedPaths = (valueWithContext.resolvedValue as Map).values.toList()
+          as List<String>;


I wonder if a spread formats nicer? Something like

resolvedPaths = [ ...(valueWithContext.resolvedValue as Map).values, ]

Not sure if there is a type error there, or if it actually would format nicer...

Ah yes, spread format is win here, good call

srawlins · 2021-08-19T16:47:50Z

lib/src/dartdoc_options.dart

+  DartdocOptionRoot(String name, ResourceProvider resourceProvider)
+      : super(name, resourceProvider);
+
+  late final ArgParser __argParser =


Is the double underscore a pattern from before the null safety migration, or new?

It's from before. I wrote this when I was still trying to write Python in Dart. I can change.

srawlins · 2021-08-19T16:52:53Z

~~Quick question~~ Big question: have you benchmarked along the null safety migration?

I'd be curious to know if the time-to-document Flutter has changed (I doubt it would), and more curious to know if the lazy initialization changes perhaps lead to different max memory usage. I think I doubt this too because theoretically the late final laziness is exactly the same as the explicit memoization laziness, but in practice...

jcollins-g · 2021-08-19T16:56:56Z

I haven't benchmarked it yet; was planning to though when the NNBD migration is a little closer

…rt-lang#2752) * Disable one of the template checks? and add beta branch for testing * beta is the new stable * empty commit - force check reset

Co-authored-by: Janice Collins <[email protected]>

* Prepare for dartdoc 2.0.0 * Update changelog for additional bugfix

* Start the NNBD migration branch with additional github configuration and a basic "non-migration" of all libraries (#2739) * Update analyzer version to 2.0.0 * grind build for analyzer 2 * First steps toward migrating * Basic migration infrastruture for new NNBD branch * use separate workflow file for nnbd branch, for clarity * thinko * Merge master into nnbd branch (#2747) * Switch to using CompilationUnitElement.classes (#2743) * Prepare dartdoc_options for migration (#2745) * more specific imports in options * do not allow autoinitialization * dartfmt * parameterize the valueWithContext type * Use alternative constructors instead of externalizing, didn't realize the autodetect was so widely used * more subclasses * Empty commit to straighten out GitHub Actions * Convert options/logging libraries to null safety (#2748) * Update analyzer version to 2.0.0 * grind build for analyzer 2 * First steps toward migrating * Start migrating some things around the edges and see what happens * Basic migration infrastruture for new NNBD branch * use separate workflow file for nnbd branch, for clarity * thinko * Switch to using CompilationUnitElement.classes (#2743) * Ignore null safe import warnings for presubmits * Prepare dartdoc_options for migration (#2745) * more specific imports in options * do not allow autoinitialization * dartfmt * parameterize the valueWithContext type * Use alternative constructors instead of externalizing, didn't realize the autodetect was so widely used * more subclasses * Empty commit to straighten out GitHub Actions * dartfmt * update NNBD branch from head (#2758) * Switch to using CompilationUnitElement.classes (#2743) * Prepare dartdoc_options for migration (#2745) * more specific imports in options * do not allow autoinitialization * dartfmt * parameterize the valueWithContext type * Use alternative constructors instead of externalizing, didn't realize the autodetect was so widely used * more subclasses * Disable the unstable template checks and prepare for 2.14 stable. (#2752) * Disable one of the template checks? and add beta branch for testing * beta is the new stable * empty commit - force check reset * Add a blurb to the README about requiring analysis (#2753) * Allow comment references on generic typedefs (#2756) Co-authored-by: Janice Collins <[email protected]> * Prepare for dartdoc 2.0.0 (#2754) * Prepare for dartdoc 2.0.0 * Update changelog for additional bugfix Co-authored-by: Simon Binder <[email protected]> * Migrate dartdoc_options.dart (#2749) * Switch to using CompilationUnitElement.classes (#2743) * Prepare dartdoc_options for migration (#2745) * more specific imports in options * do not allow autoinitialization * dartfmt * parameterize the valueWithContext type * Use alternative constructors instead of externalizing, didn't realize the autodetect was so widely used * more subclasses * flatten * Disable the unstable template checks and prepare for 2.14 stable. (#2752) * Disable one of the template checks? and add beta branch for testing * beta is the new stable * empty commit - force check reset * Add a blurb to the README about requiring analysis (#2753) * Allow comment references on generic typedefs (#2756) Co-authored-by: Janice Collins <[email protected]> * Prepare for dartdoc 2.0.0 (#2754) * Prepare for dartdoc 2.0.0 * Update changelog for additional bugfix * review comments Co-authored-by: Simon Binder <[email protected]> * Migrate package_meta, io_utils, and utils. (#2760) * flatten * Review comments * opt-out for inheriting container * A few more libraries migrated * Migrate warnings.dart (#2787) * Migrate mustachio to null safety (#2801) * Migrate various util and small libraries (#2805) * Migrate tool_runner to NNBD (#2804) * Migrate tool runner * dartfmt * getAs -> getValueAs * comment * Migrate the renderers to NNBD (#2816) * Migrate renderers * dartfmt * Migrate most mustachio tests to null safety (#2810) * Merge main branch to NNBD (#2826) * Refactor more documentation comment generation into DocumentationComment (#2817) * Straight move * Partial * delete experiment * rebuild * remove ??= from a cut and paste * Continuing the crushing down of documentation handling (#2818) * Straight move * Partial * delete experiment * rebuild * moved computeDocumentationComment and documentationComment * Move more documentation comment handling * remove ??= from a cut and paste * Documentation comment move for combos * better as a getter * Remove all deprecated bits in preparation for next release (#2821) * Remove all deprecated bits in preparation for next release * make some things final * documentationComment can now be non-nullable (#2819) * Straight move * Partial * delete experiment * rebuild * moved computeDocumentationComment and documentationComment * Move more documentation comment handling * remove ??= from a cut and paste * Documentation comment move for combos * better as a getter * Untangle synthetic combo documentation in preparing for non-null * rebuild * documentationComment can be made non-nullable * Update test.yaml to delete sdk check from beta branch * tab conversion error * correct name * Stop testing beta in the bots (#2824) * disable beta * try switching on stable * stable doesn't work for a lot of things now either :-( * try enabling the main bot * Change "dartfmt" -> "dart format". (#2822) Co-authored-by: Janice Collins <[email protected]> Co-authored-by: Bob Nystrom <[email protected]> * Migrate more libraries to NNBD (#2827) * migrate category * Migrate model_comment_reference and the parser * Fix test failure * Migrate comment_referable and model_object_builder (#2832) * squash * fix error in filter setup * fix test * Migrate element_type to nnbd (#2834) * Migrate markdown_processor and matching_link_result to NNBD (#2835) * Migrate element_type to nnbd * Migrate markdown_processor and matching_link_result * Migrate most of lib/src/generators to nnbd (#2837) * squash * empty commit - reset actions * Migrate most of the non-ModelElement bits of lib (#2838) * squash * rebuild * Fixup hints * Migrate most of what's left in tools/ (#2840) * squash * rebuild * migrate more things * subprocess * type adjustment in grinder * comments * Canonicalize default input dir * Revert "Canonicalize default input dir" This reverts commit 5d76a51. * Just disable the test, something odd is going on with MemoryResourceProvider. * Migrate ModelElement and subclasses to NNBD (#2843) * squash * rebuild * migrate more things * subprocess * type adjustment in grinder * partial * partial 2 * no errors, ship it * clean up a lot of warnings * All warnings done so really ship it * dartfmt * close * manual changes on renderers * comments * partial * fix some errors post-migration * holy cow it passes the tests now * make mustache happier and clean up an interface * forgot to remove a fixme * Work around strange super-not-allowed error in stable * review comments * Migrate runtime renderer to NNBD (#2849) * squash from chain merges * Things seem to work now, if not completely cleanly. * Get runtime_renderer_render_test ready for NNBD strong mode, fix a few more problems * deconfuse github * add todo * Update nnbd branch for latest changes and new resources-dir option (#2867) * Stop using JavaFile. (#2856) * Add a resources-dir option (#2857) * Skip sdk-analyzer job (#2863) * Use 'index' to access the enum value index. (#2865) Co-authored-by: Sam Rawlins <[email protected]> * Re-enable sdk-analyzer job, but make it not failing (#2864) * Update new resources-dir option for null safety * Fix html generator test merge Co-authored-by: Konstantin Scheglov <[email protected]> Co-authored-by: Sam Rawlins <[email protected]> * Utilize new Object.hash function instead of quiver code (#2868) * Minor cleanup to lints and some top level files (#2872) * Cleanup categorization to simplify template generation migration (#2875) * Cleanup model_element migration and surrounding accesses (#2876) * Migrate the generated AOT renderers to null safety (#2874) * Cleanup package_graph migration (#2869) * Migrate several test files to null safety (#2871) * Migrate comment_referable tests (#2877) * Migrate tool/grind.dart to null safety (#2870) * Migrate more tests; correct other migration bits (#2880) * Correct some bad mustachio migration; migrate mustachio tests (#2879) * Migrate the testing/ packages (#2881) * Migrate most end2end tests; null safety tweaks (#2882) * Make a few more nullable fields late final (#2883) * Migrate model_test, the last file, to null safety (#2884) * Merge master branch into nnbd branch (#2885) * Stop using JavaFile. (#2856) * Add a resources-dir option (#2857) * Skip sdk-analyzer job (#2863) * Use 'index' to access the enum value index. (#2865) Co-authored-by: Sam Rawlins <[email protected]> * Re-enable sdk-analyzer job, but make it not failing (#2864) Co-authored-by: Konstantin Scheglov <[email protected]> Co-authored-by: Janice Collins <[email protected]> Co-authored-by: Simon Binder <[email protected]> Co-authored-by: Bob Nystrom <[email protected]> Co-authored-by: Parker Lougheed <[email protected]> Co-authored-by: Konstantin Scheglov <[email protected]>

jcollins-g added 3 commits August 17, 2021 09:57

Switch to using CompilationUnitElement.classes (dart-lang#2743)

a7322c2

flatten

038b59f

jcollins-g added the nnbd label Aug 18, 2021

google-cla bot added the cla: yes Google CLA check succeeded. label Aug 18, 2021

jcollins-g requested a review from bwilkerson August 18, 2021 23:43

jcollins-g marked this pull request as ready for review August 18, 2021 23:45

srawlins approved these changes Aug 19, 2021

View reviewed changes

jcollins-g and others added 9 commits August 19, 2021 11:59

Disable the unstable template checks and prepare for 2.14 stable. (da…

98b4c17

…rt-lang#2752) * Disable one of the template checks? and add beta branch for testing * beta is the new stable * empty commit - force check reset

Add a blurb to the README about requiring analysis (dart-lang#2753)

2c393eb

Allow comment references on generic typedefs (dart-lang#2756)

8460c9a

Co-authored-by: Janice Collins <[email protected]>

Prepare for dartdoc 2.0.0 (dart-lang#2754)

a4ca86f

* Prepare for dartdoc 2.0.0 * Update changelog for additional bugfix

review comments

f6a5ec7

Merge branch 'master' into nnbd-mastersync-0823

7d7f80c

Merge branch 'nnbd-mastersync-0823' into nnbd-ddoptions

5901b50

Merge branch 'nnbd' into nnbd-mastersync-0823

45b6b27

Merge branch 'nnbd-mastersync-0823' into nnbd-ddoptions

7159562

jcollins-g merged commit bcc0043 into dart-lang:nnbd Aug 23, 2021

jcollins-g deleted the nnbd-ddoptions branch August 23, 2021 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate dartdoc_options.dart #2749

Migrate dartdoc_options.dart #2749

jcollins-g commented Aug 18, 2021 •

edited

Loading

jcollins-g commented Aug 18, 2021

srawlins left a comment

srawlins Aug 19, 2021

jcollins-g Aug 19, 2021

srawlins Aug 19, 2021

srawlins Aug 19, 2021

srawlins Aug 19, 2021

jcollins-g Aug 23, 2021

srawlins Aug 19, 2021

jcollins-g Aug 19, 2021

srawlins commented Aug 19, 2021

jcollins-g commented Aug 19, 2021

Migrate dartdoc_options.dart #2749

Migrate dartdoc_options.dart #2749

Conversation

jcollins-g commented Aug 18, 2021 • edited Loading

jcollins-g commented Aug 18, 2021

srawlins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srawlins commented Aug 19, 2021

jcollins-g commented Aug 19, 2021

jcollins-g commented Aug 18, 2021 •

edited

Loading