-
Notifications
You must be signed in to change notification settings - Fork 340
Add support for creating other Flutter project templates #3004
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.
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
Add support for creating other Flutter project templates #3004
Conversation
|
This all looks good to me, go ahead! Once done, we should also update the places on the website that mention Flutter: New Project to have the updated name + new commands: |
DanTup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - two minor comments about extracting some of the repeated code. Thanks!
|
|
||
| const contents = fs.readFileSync(pubspecFile); | ||
| if (contents.indexOf(expectedString) === -1) | ||
| assert.fail(`Did not find "${expectedString}'" in the file (${pubspecFile}):\n\n${contents}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given we now essentially have four copies of this test, I think it's worth pulling out a function (just in this file) that takes in the the WorkspaceFolder (vs.workspace.workspaceFolders![4]), the expectedString, and file to check ("pubspec.yaml").
| assert.equal(json?.template === "module", true); | ||
| }); | ||
|
|
||
| it("Flutter: New Package Project can be invoked and creates trigger file", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, we could pull a function out here to avoid the duplication (the body of this test was modified recently to check the pre-population of the command, so it could change again and one copy would be easier).
DanTup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one minor nit, but otherwise LGTM!
| const contents = fs.readFileSync(mainFile); | ||
| if (contents.indexOf(expectedString) === -1) | ||
| assert.fail(`Did not find "${expectedString}'" in the sample file (${mainFile}):\n\n${contents}`); | ||
| const pubspecFile = path.join(sampleProjectFolder, "lib", "main.dart"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: variable name - this isn't a pubspec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'll rename this, as landing this now to prepare a pre-release build)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh - thanks!
This currently only implements the
moduletemplate; if it looks good, then there will additional commits implementing thepluginandpackagetemplates.Fixes #2963.