Skip to content

Break down flutter doctor validations and results #34624

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
blasten merged 6 commits into from
Jun 25, 2019
Merged

Break down flutter doctor validations and results #34624

blasten merged 6 commits into from
Jun 25, 2019

Conversation

@blasten
Copy link

@blasten blasten commented Jun 18, 2019

Description

Break down the result of flutter doctor and report the result of each validation in the usage params.

Related Issues

#26029

Tests

I added the following tests:

  • 3 unit tests in doctor_test.dart

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

jonahwilliams
jonahwilliams suggested changes Jun 18, 2019
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we should be leaking the usage implementation throughout all of the doctor code

@blasten
Copy link
Author

blasten commented Jun 18, 2019

@jonahwilliams One alternative would be to map each DoctorValidator to the corresponding result in doctor.diagnose. Maybe using the subclass of DoctorValidator as key? and then loop through each result in commands/doctor.dart and assign a usage param?

I didn't go this route because it seemed a little more complex, but I'm open to any suggestion.

However, it does seem that this feature requires each validator to have a usage param key, so that coupling exists as per the feature requirement. I don't see this as leaking implementation detail of the Usage class, but more as constructing a K/V pair with the key of the validation and the result, which happens to be used as the usage params.

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Jun 18, 2019

However, it does seem that this feature requires each validator to have a usage param key, so that coupling exists as per the feature requirement.

We should not do that because it will prevent us from making doctor extensible.

@Piinks Piinks added t: flutter doctor Problem related to the "flutter doctor" tool tool Affects the "flutter" command-line tool. See also t: labels. labels Jun 18, 2019
@blasten
Copy link
Author

blasten commented Jun 19, 2019

I incorporated the feedback based on the discussion. PTAL

@blasten blasten requested a review from jonahwilliams June 19, 2019 00:12
jonahwilliams
jonahwilliams reviewed Jun 20, 2019
View reviewed changes
@blasten
Copy link
Author

blasten commented Jun 25, 2019

PTAL

tvolkert
tvolkert approved these changes Jun 25, 2019
View reviewed changes
Copy link
Contributor

@tvolkert tvolkert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jonahwilliams
jonahwilliams approved these changes Jun 25, 2019
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@blasten blasten merged commit 2cf8213 into flutter:master Jun 25, 2019
@blasten blasten deleted the log_doctor branch June 25, 2019 17:22
johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
@johnsonmh
Break down flutter doctor validations and results (flutter#34624)
b02d64a
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@christopherfujino christopherfujino Awaiting requested review from christopherfujino

@zanderso zanderso Awaiting requested review from zanderso

3 more reviewers

@jayoung-lee jayoung-lee jayoung-lee left review comments

@jonahwilliams jonahwilliams jonahwilliams approved these changes

@tvolkert tvolkert tvolkert approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

t: flutter doctor Problem related to the "flutter doctor" tool tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@blasten @jonahwilliams @zanderso @jayoung-lee @tvolkert @googlebot @Piinks