Create categories for doctor validators #20758

tonyzhao1 · 2018-08-17T22:38:40Z

Defines categories for doctor validators. Modifies diagnose() and summary() to group output based on category for applicable categories. This change should not affect diagnose() and summary() output.

Grouping is currently disabled and will be enabled on a per-category basis in future changes. (This ensures that IDE validators aren't incorrectly grouped together.)

Note: The commit "Change ValidatorCategory to handle..." contains the changes to diagnose() but the commit message does not reflect this.

…dators.

This reverts commit 05c2880.

category (e.g. IntelliJ). Also make Android Studio and Android toolchain use separate categories. At this stage, flutter doctor output matches what it was previously. (The summary() method itself has not yet been changed )

Output is still unchanged.

needed.

muddying the pull.

# Conflicts: # bin/internal/engine.version

tonyzhao1 · 2018-08-27T18:52:42Z

@mehmetf @tvolkert

mehmetf

Good first cut! Some comments.

mehmetf · 2018-08-28T03:21:54Z

packages/flutter_tools/lib/src/android/android_workflow.dart

-class AndroidWorkflow extends DoctorValidator implements Workflow {
-  AndroidWorkflow() : super('Android toolchain - develop for Android devices');
+class AndroidWorkflow implements Workflow {
+  AndroidWorkflow();


You can remove the constructor.

mehmetf · 2018-08-28T03:22:13Z

packages/flutter_tools/lib/src/context_runner.dart

      AndroidSdk: AndroidSdk.locateAndroidSdk,
      AndroidStudio: AndroidStudio.latestValid,
      AndroidWorkflow: () => new AndroidWorkflow(),
+      AndroidValidator: () => new AndroidValidator(),


What's the difference between a validator and a workflow?

Currently, the workflow just checks whether we should run the validators associated with it.

I thought it'd be strange to split a validator and have only one of them with the workflow attached. If it'd be better to keep the workflow on the validator, I don't mind undoing this part of the change.

I didn't mean for you to undo this change but it would be good to have a better explanation of Validator vs Workflow in comments.

So a workflow is just a collection of validators then?

This is confusing because down below, you have a list of workflows and a list of validators in the same class. Are these just validators that don't belong to any workflow?

I'm more or less breaking the old, implicit relationship where a workflow is-a validator (both workflow classes were also validators and DoctorValidatorProvider also made that assumption), so they're now two totally separate classes. A validator is still a validator (checks for installed software and generates doctor output), while a workflow checks if the environment is one where we can list or launch devices.

The only relation between validators and workflows now is that we use Workflow.appliesToHostPlatform to decide whether to add a validator to the validator list (skipping the iOS validator if we're not on Mac).

The validators in the provider are effectively the same as they used to be. The new workflows list is there because I can no longer have the validators list do double duty, so I'm explicitly listing them instead of using the old code that got them implicitly by checking which validators were also workflows.

Workflow might be a bad name for it now, since the class scope is a lot smaller now that the validator methods have been pulled out. But I don't know what would be a good, unconfusing name.

Got it. Thanks for the context.

mehmetf · 2018-08-28T03:23:54Z

packages/flutter_tools/lib/src/doctor.dart

+      ValidationResult result;
+
+      if (currentCategory.isGrouped) {
+        if (finishedGroups.contains(currentCategory)) {


When could this happen? Should this be an assert instead?

I meant to add to finishedGroups after handling a grouped category.

What I meant was whether this is a natural occurrence or not. If the validators are grouped properly and each group executes once, why would we ever see a finished group? If this is valid case, please explain it as a comment above "continue". If it is not a valid case, please assert it (i.e. throw IllegalStateException if it happens).

We're iterating over validators rather than categories. The first time we encounter a category, we skip ahead and round up all the output associated with that category. But we don't modify the validators list at all, so we'll continue to encounter that category (once for each validator in it).

mehmetf · 2018-08-28T03:26:06Z

packages/flutter_tools/lib/src/doctor.dart

+                (DoctorValidator v) => v.category == currentCategory)) {
+          results.add(await subValidator.validate());
+        }
+        result = _mergeValidationResults(results);


Where is finishedGroups updated?

packages/flutter_tools/lib/src/doctor.dart

mehmetf · 2018-08-28T03:28:16Z

packages/flutter_tools/lib/src/doctor.dart

+  static const ValidatorCategory flutter = ValidatorCategory('flutter', false);
+  static const ValidatorCategory ide = ValidatorCategory('ide', false);
+  static const ValidatorCategory device = ValidatorCategory('device', false);
+  static const ValidatorCategory other = ValidatorCategory('other', false);


All grouped is false. Why do we need isGrouped then?

Changed android and ios to true, though since the validators aren't split yet, the true/false value doesn't visibly change anything.

mehmetf · 2018-08-28T03:29:21Z

@tvolkert Since this is tooling, please review when you get the chance. This is an important step in making doctor more extensible.

tvolkert

Tests?

packages/flutter_tools/lib/src/doctor.dart

tvolkert · 2018-08-28T04:35:29Z

packages/flutter_tools/lib/src/doctor.dart

+  // Whether we should bundle results for validators sharing this cateogry,
+  // or let each stand alone.
+  final bool isGrouped;
+  const ValidatorCategory(this.name, this.isGrouped);


Does this need to be public?

I used it to create fake grouped categories in the (newly added) tests.

merging.

# Conflicts: # packages/flutter_tools/lib/src/doctor.dart

DanTup · 2018-09-05T09:35:58Z

packages/flutter_tools/lib/src/doctor.dart

+
+  @override
+  List<Workflow> get workflows {
+    _workflows ??= <Workflow>[iosWorkflow, androidWorkflow];


FYI - I had to change this code slightly because it introduced an issue on Windows (#21418). I've expanded this to work like the method above it, and only add the workflows to the list if they apply to the host platform.

PR: #21423

tonyzhao1 and others added 7 commits August 10, 2018 11:18

First step in Flutter Doctor refactor. Assigns categories to all vali…

1889419

…dators.

Revert "Roll engine e54bc4e..a84b210 (6 commits) (flutter#20453)"

312e9c9

This reverts commit 05c2880.

Split iOS and Android workflows into workflow and validator classes.

91ae944

Change doctor summary code to support validator categories.

f587aee

Output is still unchanged.

Handle small formatting issues.

512fc71

Flip Flutter category's isGroup field to false until it's actually

5564413

needed.

googlebot added the cla: yes label Aug 17, 2018

tonyzhao1 and others added 6 commits August 20, 2018 10:18

Merge branch 'master' into master

ac6b9f8

Revert auto-generated formatting changes to keep those lines from

862f7cc

muddying the pull.

Merge remote-tracking branch 'remotes/upstream/revert-20453-roll_branch'

fc8bf24

Merge remote-tracking branch 'origin/master'

7d85c27

# Conflicts: # bin/internal/engine.version

Small fixes pointed out by analyzer.

56ba71e

Properly fix analyzer issues around const constructors.

bd1c6f8

mehmetf reviewed Aug 28, 2018

View reviewed changes

tvolkert reviewed Aug 28, 2018

View reviewed changes

tonyzhao1 and others added 5 commits August 31, 2018 13:21

Small changes to address comments.

efbaff2

Add tests to verify grouped validator behavior and validationtype

d0563f6

merging.

Update doctor.dart

31869ce

Add comments for clarification.

4dc92ac

Merge remote-tracking branch 'origin/master'

d9a202c

# Conflicts: # packages/flutter_tools/lib/src/doctor.dart

mehmetf approved these changes Sep 5, 2018

View reviewed changes

mehmetf merged commit 58d98ce into flutter:master Sep 5, 2018

DanTup mentioned this pull request Sep 5, 2018

Fix windows crash running flutter run which tries to find Xcode #21423

Merged

DanTup reviewed Sep 5, 2018

View reviewed changes

github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021

Create categories for doctor validators #20758

Create categories for doctor validators #20758

Uh oh!

Conversation

tonyzhao1 commented Aug 17, 2018

Uh oh!

tonyzhao1 commented Aug 27, 2018

Uh oh!

mehmetf 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mehmetf commented Aug 28, 2018

Uh oh!

tvolkert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants