Skip to content

Add --package option to create command. - #11105

Merged
jakobr-google merged 4 commits into
flutter:masterfrom
jakobr-google:package
Aug 23, 2017
Merged

jakobr-google merged 4 commits into
flutter:masterfrom
jakobr-google:package

Conversation

@jakobr-google

Copy link
Copy Markdown
Contributor

Creates a Flutter package, which is a package that depends on Flutter, but does not contain native code.

Fixes #10377.

@devoncarew devoncarew 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.

I added some drive my comments about the cli ux -

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.

We should have a bit more verbose description here, to help the user choose between a regular project, and plugin project, and a package project.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can use the following help strings:

Package: "Generate a shareable Flutter project containing modular Dart code."

Plugin: "Generate a shareable Flutter project containing an API in Dart code with a platform-specific implementation for Android, for iOS code, or for both."

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.

I think we should switch to using an option to select project types; something like --template=<type>, where type is one of application, package, or plugin. That feels a bit cleaner to me than having several different, mutually exclusive, cli flags.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that is a good idea, but it would be a breaking change impacting docs, code labs, etc. so we should handle that as a separate change later.

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.

We can do it in this patch and just provide a legacy --plugin option to not break people in the meantime.

The thing to avoid is adding something we know we're going to change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer flutter create package over flutter create --template=package, if we can make this work with the argument parser (ideally defaulting to 'application' if a sub-command to create isn't specified).

I personally think having the shorter flags is cleaner and easier to use than the longer --template=.... Note that we have mutually exclusive flags elsewhere, too -- the build mode flags, for example.

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.

The first rest argument is currently used for the output location: flutter create <output directory>, so I think it'd be problematic to use it for the project type as well.

I personally think having the shorter flags is cleaner and easier to use than the longer --template=...

Gotcha. The length may not be critical here, since flutter create a less common command (compared to something like flutter run). I think I still prefer the option flag (--template=foo) - I do think it makes it more clear that you're selecting between a few different project templates.

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.

My main concern with having multiple flags is that then we have to handle the cases of them being specified together (--plugin --package), which gets confusing to explain (you can use these together, but not these, etc), especially since we have other flags that do work in combination.

--debug, --profile, and --release are an unfortunate set of values that we've grandfathered in because that's what people expect coming from other build systems, but I don't think that's a problem here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Missing a line feed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This was fixed!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't think we need these plugin details (lines 8-10)?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This was fixed

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.

nit: syntax of todo

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.

we definitely don't want to be including any default licenses with any of the flutter create output

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Yeah. Can you do that in this PR also?

We should probably just have placeholder LICENSE files so that people see that they should fill it in. e.g. just a TODO.

@Hixie

Hixie commented Aug 2, 2017

Copy link
Copy Markdown
Contributor

@jakobr-google Are you still working on this PR?

@jakobr-google

Copy link
Copy Markdown
Contributor Author

Yep, I'll pick it up again. Was struck by vacation.

@mit-mit

mit-mit commented Aug 15, 2017

Copy link
Copy Markdown
Member

LGTM

@mit-mit

mit-mit commented Aug 15, 2017

Copy link
Copy Markdown
Member

But the test is failing:

04:23 +157 -1: test/commands/create_test.dart: create package project [E]
  ProcessException: No such file or directory
    Command: /home/travis/build/flutter/flutter/bin/cache/dart-sdk/bin/dart 

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.

what is a Calculator?

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.

oh i see never mind.

@mit-mit

mit-mit commented Aug 22, 2017

Copy link
Copy Markdown
Member

@jakobr-google is this ready to go in?

Creates a Flutter package, which is a package that depends on Flutter, but does not contain native code.

Fixes flutter#10377.
@jakobr-google
jakobr-google merged commit 5d0d612 into flutter:master Aug 23, 2017
@jakobr-google
jakobr-google deleted the package branch August 23, 2017 11:29
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support flutter create --package

5 participants