Skip to content

[flutter_tool] process.dart cleanup - #39899

Merged
zanderso merged 1 commit into
flutter:masterfrom
zanderso:process-cleanup
Sep 12, 2019
Merged

[flutter_tool] process.dart cleanup#39899
zanderso merged 1 commit into
flutter:masterfrom
zanderso:process-cleanup

Conversation

@zanderso

@zanderso zanderso commented Sep 5, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Description

Prior to this PR, base/process.dart contained 8-ish top-level wrappers of ProcessManager methods. These wrappers aren't strictly necessary since call-sites could instead just use the ProcessManager calls, but they simplify some common tasks, so I'd like to keep them.

This PR groups these wrappers under an inject-able class ProcessUtils, and removes the *Checked* versions, replacing them with passing the throwsOnError optional named parameter to the remaining un-checked versions.

Bookmark to process.dart in the PR: https://github.com/flutter/flutter/pull/39899/files#diff-22b5beed6542f4fbfb2dab53913549d7. The rest of the PR is compensating changes for the refactor.

Tests

I added the following tests:

Added tests of wrappers to process_test.dart

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.

@zanderso zanderso added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Sep 5, 2019
@codecov

codecov Bot commented Sep 5, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@c5e2bf5). Click here to learn what that means.
The diff coverage is 65.78%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #39899   +/-   ##
=========================================
  Coverage          ?   59.76%           
=========================================
  Files             ?      192           
  Lines             ?    18601           
  Branches          ?        0           
=========================================
  Hits              ?    11117           
  Misses            ?     7484           
  Partials          ?        0
Flag Coverage Δ
#flutter_tool 59.76% <65.78%> (?)
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/base/build.dart 67.5% <0%> (ø)
...tools/lib/src/fuchsia/fuchsia_kernel_compiler.dart 7.69% <0%> (ø)
packages/flutter_tools/lib/src/commands/drive.dart 75% <0%> (ø)
.../flutter_tools/lib/src/android/android_studio.dart 50% <0%> (ø)
...ages/flutter_tools/lib/src/fuchsia/fuchsia_pm.dart 27.77% <0%> (ø)
...ages/flutter_tools/lib/src/commands/build_aot.dart 39.79% <0%> (ø)
...tter_tools/lib/src/fuchsia/fuchsia_dev_finder.dart 0% <0%> (ø)
...ackages/flutter_tools/lib/src/commands/unpack.dart 4.95% <0%> (ø)
...flutter_tools/lib/src/test/coverage_collector.dart 61.42% <0%> (ø)
packages/flutter_tools/lib/src/base/os.dart 31.78% <0%> (ø)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5e2bf5...5588e46. Read the comment docs.

jonahwilliams
jonahwilliams approved these changes Sep 10, 2019
View reviewed changes

@jonahwilliams jonahwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Yay for fewer ways to do the same thing!

blasten
blasten approved these changes Sep 10, 2019
View reviewed changes

@blasten blasten left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice cleanup!

@zanderso
zanderso merged commit 73c10e8 into flutter:master Sep 12, 2019
@zanderso
zanderso deleted the process-cleanup branch September 12, 2019 01:20
@orischwartz orischwartz mentioned this pull request Sep 13, 2019
Closed
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
@zanderso @Inconnu08
[flutter_tool] process.dart cleanup (flutter#39899)
22bd1ca
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 4, 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
2 more reviewers
@blasten blasten blasten approved these changes
@jonahwilliams jonahwilliams jonahwilliams approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

4 participants

@zanderso @blasten @jonahwilliams @googlebot