Support create java function project & add java function #66

jdneo · 2017-11-10T08:27:41Z

This PR add the following feature:

Create java function project ( Support Create Java Function Project in extension #48 )
Add java function ( Support add Java Function into Java Function Project #49 )

For creating java function project, we leverage maven archetype.

For adding java function, we leverage azure function maven plugin. The reason we call it in terminal is that the maven plugin does not support batch mode for this maven goal.

As to the tasks.json file for Java function, we do not use dependOn for tasks because this may cause preLaunchTask cannot be tracked error in VS Code 1.18 (see issue here).

------------- Update on Wed Nov 15 2017 09:12:33 GMT+0800 -------------
Remove the add Java function part, since the Maven plugin not support adding java function non-interactively.

ejizba

What's the plan for create Function? If the user has to interact with the terminal, then I don't think the behavior is ship-able. We should at least display quick picks for the type of template

ejizba · 2017-11-10T23:45:32Z

src/utils/workspace.ts

@@ -32,3 +35,15 @@ export function isFolderOpenInWorkspace(fsPath: string): boolean {
        return false;
    }
 }
+
+export function getProjectType(projectPath: string): string {


This file is for 'workspace' utilities, but this function doesn't use vscode.workspace at all. Just move it into 'createFunction' (the only place it's used)

I think I'll use this method again for deploying Java functions. How about making a new file called 'project.ts' and move to it?

ejizba · 2017-11-10T23:46:47Z

src/utils/executor.ts

+    terminal.sendText(command, addNewLine);
+}
+
+export async function executeCommand(outputChannel: vscode.OutputChannel, workingDirectory: string, command: string, ...args: string[]): Promise<void> {


Use cpUtils.executeCommand instead

ejizba · 2017-11-11T00:10:33Z

src/commands/createNewProject.ts

+                '-B' // in Batch Mode
+            );
+
+            functionAppPath = path.join(functionAppPath, artifactId);


I thought you were going to file an issue for this? It's really annoying and we need to make sure we fix it before release

Do you mean the extra folder of artifactId?

ejizba · 2017-11-13T21:32:35Z

src/commands/createNewProject.ts

+            break;
+        default:
+            // the maven archetype contains these files, so not check them when language is Java
+            const hostJsonPath: string = path.join(functionAppPath, 'host.json');


Why does the maven archetype have these files? Shouldn't it let VS Code (or the func cli) handle it? Same with the .gitignore

Do you own the maven archetype or does someone else? I thought you guys created it

ejizba · 2017-11-13T21:33:08Z

src/utils/executor.ts

+const terminal: vscode.Terminal = vscode.window.createTerminal('Azure Functions');
+
+export function runCommandInTerminal(command: string, addNewLine: boolean = true): void {
+    terminal.show();


We don't like using terminal because then you have no idea when the command finishes

ejizba · 2017-11-13T21:36:17Z

src/utils/workspace.ts

+
+export function getProjectType(projectPath: string): string {
+    let language: string = TemplateLanguage.JavaScript;
+    fse.readdirSync(projectPath).forEach((file: string) => {


You should use async methods wherever possible (in this case, you would use readdir instead of readdirsync

Using fse.pathExists().

ejizba · 2017-11-13T21:36:29Z

src/utils/workspace.ts

+    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.


Isn't pom.xml at the root of the project? Couldn't you just do fse.pathExists(path.join(projectPath, 'pom.xml'))?

If it's not always at the root, then we need to find a different way to detect Java projects - iterating through an entire folder has bad performance for the user

Yes it's at the root path. I like the idea of using fse.pathExists()

ejizba · 2017-11-13T21:38:44Z

test/createNewProject.test.ts

+    assert.equal(inputs.length, 0, 'Not all inputs were used.');
+}
+
+async function testInitFileExist(projectPath: string): Promise<void> {


nit: testProjectFilesExist

(I don't know what InitFile means)

Make sense.

ejizba · 2017-11-13T21:49:33Z

src/commands/createFunction.ts

-
-    if (template.bindingType !== 'httpTrigger') {
-        await localAppSettings.validateAzureWebJobsStorage();
+    const languageType: string = workspaceUtil.getProjectType(functionAppPath);


Why do you infer the project type? I can easily create C# and JavaScript functions in the same project from the command line today.

Shouldn't I be able to create 'Java' and 'JavaScript' functions in the same project from VS Code?

How about let the user choose the language he/she wants to add by showing Picks?

ejizba · 2017-11-14T23:31:21Z

src/utils/executor.ts

+
+import * as vscode from 'vscode';
+
+const terminal: vscode.Terminal = vscode.window.createTerminal('Azure Functions');


If the user closes/deletes the 'Azure Functions' terminal, then create new project will fail every time until they restart VS Code

ejizba · 2017-11-15T00:01:28Z

src/commands/createNewProject.ts

+                '-B' // in Batch Mode
+            );
+
+            functionAppPath = path.join(functionAppPath, artifactId);


ejizba

I still think running in a terminal is a feature-blocker. Sent an email offline.

In the future, it would be better to do one PR for each feature. I'm sorry connect(); distracted us from getting this in last week, but I think the 'Create new Java Project' feature is good-to-go and it's just being blocked by issues with 'Create new Java function'

support create java function project & add java function

5dbd727

jdneo mentioned this pull request Nov 10, 2017

Support create java function project #51

Closed

not check gitignore when language is Java

2619208

jdneo force-pushed the new-pr branch from 235b538 to 2619208 Compare November 13, 2017 00:56

UT for create new project

3a12a5a

fiveisprime requested a review from a team November 13, 2017 15:01

ejizba suggested changes Nov 13, 2017

View reviewed changes

modify according to comments

3afe9ab

ejizba reviewed Nov 15, 2017

View reviewed changes

remove Java function add part

3ce8a4a

ejizba approved these changes Nov 15, 2017

View reviewed changes

ejizba merged commit 28b0620 into microsoft:master Nov 15, 2017

jdneo deleted the new-pr branch February 5, 2018 09:26

github-actions bot locked and limited conversation to collaborators Jan 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support create java function project & add java function #66

Support create java function project & add java function #66

jdneo commented Nov 10, 2017 •

edited

Loading

ejizba left a comment

ejizba Nov 10, 2017

jdneo Nov 14, 2017 •

edited

Loading

ejizba Nov 10, 2017

jdneo Nov 14, 2017

ejizba Nov 11, 2017

jdneo Nov 14, 2017

ejizba Nov 15, 2017

ejizba Nov 15, 2017

ejizba Nov 13, 2017

ejizba Nov 15, 2017

ejizba Nov 13, 2017

ejizba Nov 13, 2017

jdneo Nov 14, 2017

ejizba Nov 13, 2017

jdneo Nov 14, 2017

ejizba Nov 13, 2017

jdneo Nov 14, 2017

ejizba Nov 13, 2017

jdneo Nov 14, 2017

ejizba Nov 14, 2017

ejizba Nov 15, 2017

ejizba left a comment


		import * as vscode from 'vscode';

		const terminal: vscode.Terminal = vscode.window.createTerminal('Azure Functions');

Support create java function project & add java function #66

Support create java function project & add java function #66

Conversation

jdneo commented Nov 10, 2017 • edited Loading

ejizba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdneo Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejizba left a comment

Choose a reason for hiding this comment

jdneo commented Nov 10, 2017 •

edited

Loading

jdneo Nov 14, 2017 •

edited

Loading