flutter_tools: refactor `CreateCommand`. by cyanglaz · Pull Request #70874 · flutter/flutter

cyanglaz · 2020-11-19T21:28:08Z

Description

This PR refactors some methods form CreateCommand to a mixin. This is to get ready for making flutter create plugin a separate command. (#59493)

This refactoring does the bare minimum in order for the new flutter create plugin to reuse the code in CreateCommand.
So it is basically a copy paste to make the private method reusable by a separate class.

Related Issues

#59493
Fixes #59494

Tests

I added the following tests:

This is a pure refactoring, nothing new was added.

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 read the Tree Hygiene wiki page, which explains my responsibilities.
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

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

flutter-dashboard · 2020-11-19T21:28:11Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

jonahwilliams

I would start with a base class over a mixin

jonahwilliams · 2020-11-19T22:41:17Z

packages/flutter_tools/lib/src/commands/create_utils.dart

+/// Mixin contains methods that are shared with `flutter create` commands.
+mixin CreateCommandMixin on FlutterCommand {
+  /// Throw with exit code 2 if the output directory is invalid.
+  void validateOutoutDirectoryArg() {


jonahwilliams · 2020-11-19T22:41:33Z

packages/flutter_tools/lib/src/commands/create_utils.dart

+        !_keywords.contains(name);
+  }
+
+  /// Generate application project in the `directory` using `templateCnotext`.


templateContext

jonahwilliams · 2020-11-19T22:42:34Z

packages/flutter_tools/lib/src/commands/create_utils.dart

+import '../template.dart';
+
+/// Mixin contains methods that are shared with `flutter create` commands.
+mixin CreateCommandMixin on FlutterCommand {


Doesn't seem like there is a good reason for this to be a mixin. I would instead make this CreateBase and an abstract class, and then rename the library to create_base.dart.

jonahwilliams · 2020-11-19T22:43:03Z

packages/flutter_tools/lib/src/commands/create_utils.dart

+  String getFlutterRoot() {
+    if (Cache.flutterRoot == null) {
+      throwToolExit(
+          'Neither the --flutter-root command line flag nor the FLUTTER_ROOT environment '


Remove the section about --flutter-root

Suggested change

'Neither the --flutter-root command line flag nor the FLUTTER_ROOT environment '

'Neither the --flutter-root command line flag nor the FLUTTER_ROOT environment '

jonahwilliams · 2020-11-19T22:43:30Z

packages/flutter_tools/lib/src/commands/create_utils.dart

+  /// Throws assertion if projectDir does not exist or empty.
+  /// Returns null if no project type can be determined.
+  FlutterProjectType determineTemplateType(Directory projectDir) {
+    assert(projectDir.existsSync() && projectDir.listSync().isNotEmpty);


asserts only run during tests, do these need to be error checks?

This method only makes sense when projectDir is not empty. So I think it makes sense to have an assertion here so bugs can be easier determined in the future. (And yes, it would only help debugging under tests)

jonahwilliams · 2020-11-19T22:44:09Z

packages/flutter_tools/lib/src/commands/create.dart


-class CreateCommand extends FlutterCommand {
+class CreateCommand extends FlutterCommand with CreateCommandMixin {
  CreateCommand() {


If you make CreateBase an abstract class, then you should move the shared argParser options to that class

cyanglaz

@jonahwilliams Updated. PTAL

cyanglaz · 2020-11-19T23:03:35Z

packages/flutter_tools/lib/src/commands/create_utils.dart

+import '../template.dart';
+
+/// Mixin contains methods that are shared with `flutter create` commands.
+mixin CreateCommandMixin on FlutterCommand {


cyanglaz · 2020-11-19T23:08:03Z

packages/flutter_tools/lib/src/commands/create_utils.dart

+  /// Throws assertion if projectDir does not exist or empty.
+  /// Returns null if no project type can be determined.
+  FlutterProjectType determineTemplateType(Directory projectDir) {
+    assert(projectDir.existsSync() && projectDir.listSync().isNotEmpty);


This method only makes sense when projectDir is not empty. So I think it makes sense to have an assertion here so bugs can be easier determined in the future. (And yes, it would only help debugging under tests)

cyanglaz · 2020-11-19T23:17:28Z

packages/flutter_tools/lib/src/commands/create.dart


-class CreateCommand extends FlutterCommand {
+class CreateCommand extends FlutterCommand with CreateCommandMixin {
  CreateCommand() {


jonahwilliams

Overall LGTM.

I'd let @jmagman take a look too

jonahwilliams · 2020-11-20T00:01:31Z

packages/flutter_tools/lib/src/commands/create_base.dart

+  void addPlatformsOptions({String customHelp}) {
+    argParser.addMultiOption('platforms',
+        help: customHelp ??
+            'Required: The platforms supported by this project. '


nit: make this a constant

jonahwilliams

Also please fix the tests

jmagman

LGTM with nits once the tests are fixed.

jmagman · 2020-11-20T01:17:39Z

packages/flutter_tools/lib/src/commands/create_base.dart

+  ///
+  /// If `--org` is specified in the command, returns that directly.
+  /// If `--org` is not specified, returns the organization from the existing project.
+  Future<String> getOrganization(Directory projectDir) async {


Make a getter?

jmagman · 2020-11-20T01:17:46Z

packages/flutter_tools/lib/src/commands/create_base.dart

+  /// Gets the project name based.
+  ///
+  /// Use the current directory path name if the `--project-name` is not specified explicitly.
+  String getProjectName(String projectDirPath) {


Make a getter?

jmagman · 2020-11-20T01:19:20Z

packages/flutter_tools/lib/src/commands/create_base.dart

+    return camelizedName[0].toUpperCase() + camelizedName.substring(1);
+  }
+
+  String createUTIIdentifier(String organization, String name) {


Can these be made @protected?

jmagman · 2020-11-20T18:27:18Z

Overall LGTM.

@jonahwilliams can you approve?

jonahwilliams

LGTM

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 19, 2020

google-cla bot added the cla: yes label Nov 19, 2020

refactor CreateCommand

71d3abd

cyanglaz force-pushed the create_refactor branch from 997ee43 to 71d3abd Compare November 19, 2020 21:29

format

c9d8e7d

cyanglaz requested review from amirh, jmagman and jonahwilliams November 19, 2020 21:30

jonahwilliams suggested changes Nov 19, 2020

View reviewed changes

review

d6e79cc

cyanglaz commented Nov 19, 2020

View reviewed changes

jonahwilliams reviewed Nov 20, 2020

View reviewed changes

jmagman approved these changes Nov 20, 2020

View reviewed changes

Chris Yang added 4 commits November 20, 2020 09:17

review

c11fe94

fix ci

420300b

ci

7445189

line EOF

fb12f43

jonahwilliams approved these changes Nov 20, 2020

View reviewed changes

cyanglaz added the waiting for tree to go green label Nov 20, 2020

fluttergithubbot merged commit 4e88f6a into flutter:master Nov 20, 2020

cyanglaz deleted the create_refactor branch November 20, 2020 19:06

cyanglaz mentioned this pull request Nov 20, 2020

flutter_tools: do more refactor on CreateBase and CreateCommand #70959

Merged

13 tasks

oshtman mentioned this pull request May 19, 2021

Failing iOS release builds - Target aot_assembly_release failed: Exception: AOT snapshotter exited with code -11 #82890

Closed

	'Neither the --flutter-root command line flag nor the FLUTTER_ROOT environment '
	'Neither the --flutter-root command line flag nor the FLUTTER_ROOT environment '

Conversation

cyanglaz commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Nov 19, 2020

Uh oh!

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

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

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

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

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

jmagman commented Nov 20, 2020

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cyanglaz commented Nov 19, 2020 •

edited

Loading