-
Notifications
You must be signed in to change notification settings - Fork 136
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
Support create java function project #51
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
/*--------------------------------------------------------------------------------------------- | ||
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
*--------------------------------------------------------------------------------------------*/ | ||
|
||
import * as vscode from 'vscode'; | ||
|
||
const terminal: vscode.Terminal = vscode.window.createTerminal('Azure Functions'); | ||
|
||
// tslint:disable-next-line:export-name | ||
export function runCommandInTerminal(command: string, addNewLine: boolean = true): void { | ||
terminal.show(); | ||
terminal.sendText(command, addNewLine); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,10 +3,12 @@ | |
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
*--------------------------------------------------------------------------------------------*/ | ||
|
||
import * as fse from 'fs-extra'; | ||
import * as path from 'path'; | ||
import * as vscode from 'vscode'; | ||
import { IUserInterface, PickWithData } from '../IUserInterface'; | ||
import { localize } from '../localize'; | ||
import { TemplateLanguage } from '../templates/Template'; | ||
|
||
export async function selectWorkspaceFolder(ui: IUserInterface, placeholder: string): Promise<string> { | ||
const browse: string = ':browse'; | ||
|
@@ -23,16 +25,25 @@ export async function selectWorkspaceFolder(ui: IUserInterface, placeholder: str | |
|
||
export function isFolderOpenInWorkspace(fsPath: string): boolean { | ||
if (vscode.workspace.workspaceFolders) { | ||
if (!fsPath.endsWith(path.sep)) { | ||
fsPath = fsPath + path.sep; | ||
} | ||
|
||
const folder: vscode.WorkspaceFolder | undefined = vscode.workspace.workspaceFolders.find((f: vscode.WorkspaceFolder): boolean => { | ||
return fsPath.startsWith(f.uri.fsPath); | ||
return path.relative(fsPath, f.uri.fsPath) === ''; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm - this definitely fixes some edge cases, but it's not quite what I originally intended the behavior to be. This function should return true even if the path is a subfolder of a workspace. Maybe something like this would work?:
This logic can be tricky with lots of edge cases, so I'd prefer to add some unit tests when we fix this. You can do that in this PR or file an issue to fix this separately (I don't mind fixing this myself) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My concern is if the subfolder is not opened, the user might not able to launch it since the cwd might not have Another concern is that as what we discussed above. The maven archetype will always create a artifactId folder first, and put function things into the folder. For Java, there is another important file: I quite agree that always open the project base path might not be the best solution for this. While I think dong this it not that harmful. What about keeping this change for now and file a issue and we can do more further discussion in detail? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah filing an issue is fine There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great |
||
}); | ||
|
||
return folder !== undefined; | ||
} else { | ||
return false; | ||
} | ||
} | ||
|
||
export function getProjectType(projectPath: string): string { | ||
let language: string = TemplateLanguage.JavaScript; | ||
fse.readdirSync(projectPath).forEach((file: string) => { | ||
const stat: fse.Stats = fse.statSync(path.join(projectPath, file)); | ||
// Currently checking the existing pom.xml to determine whether the function project is Java language based. | ||
if (stat.isFile() && file === 'pom.xml') { | ||
language = TemplateLanguage.Java; | ||
} | ||
}); | ||
return language; | ||
} |
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.
The user explicitly selected the functionAppPath above. I don't think we should change it after the fact
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.
This is because java function project that generated by the Maven archetype is different from JS function in terms of the folder structure.
For example, this is what it looks like when we use
func init
to create a function projectAnd this is what it looks like to use maven function archetype:
So the reason why join the path when it's a java function project is that we want to open the folder that contains the function related files (
host.json
,local.settings.json
, etc.) also thepom.xml
which is java specific.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.
The above examples look like the same folder structure to me. It's just that in the top one the user selected
D:\
as the functionAppPath and in the bottom one the user selectedD:\myfunction
as the functionAppPath. (I understand thatfunc init
andmvn
deal with this slightly differently, but the user doesn't care about that internal detail. They just want the same behavior in both scenarios)With your current code, I end up with an extra folder every time:
I'm never going to put anything else in 'myFunctionProject'. It seems like rather than asking for the artifactId, you should just infer it from the parent directory name
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.
Yes, what I mean is that the
myfunction
is the extra folder generated by the maven archetype. (In your case it's the folder:artifactId
). The functionAppPath user selected in both senario is the same. Unfortunately, the maven archetype will always create the extra folder for the user.How about treat it as you said, infer artifactId from the parent directory name, without asking user for it. And for Java function project, we still keep the path join logic here to ensure that the extension will open the right base folder for function project?
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.
What if you run the maven command from the parent folder - with the folder name as the artifactId? Wouldn't that solve the issue?
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.
Fail with what error?
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.
Here is the screenshot:
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.
BTW, the error msg ouput by Maven is not that meaningful.
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.
Alright - please file an issue for this. I don't see an obvious solution, but I still think it's worth further investigation later
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.
Yes. I agree with you. I'll file an issue later.