Skip to content

Open liberty starter#112

Open
Jonathan-Maciel wants to merge 9 commits into
mainfrom
open-liberty-starter
Open

Open liberty starter#112
Jonathan-Maciel wants to merge 9 commits into
mainfrom
open-liberty-starter

Conversation

@Jonathan-Maciel

Copy link
Copy Markdown

No description provided.

Comment thread package.json Outdated
Comment thread package.json Outdated
Comment thread src/liberty/multiStepInput.ts Outdated
Comment thread src/liberty/multiStepInput.ts Outdated
import * as vscode from "vscode";
import * as fs from "fs";

export async function multiStepInput(context: ExtensionContext) {

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.

This class uses tabs for indents but starterOptions.ts uses 4 spaces for indents. To keep consistent let's keep using 4 spaces for indents. Some existing classes may need updating to keep consistent, offhand I noticed libertyProject.ts

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.

Suggestion to add some JSDoc style comments to explain new classes/functions

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

Please merge the latest from main to this branch

import * as vscode from "vscode";
import * as fs from "fs";

export async function starterProject(context: ExtensionContext) {

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.

Indentation looks off in this file, ensure you are using 4 spaces rather than tabs.
Suggestion to add a comment to the class explaining the purpose

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in fb3698a. Comments on the way..

Comment thread src/liberty/devCommands.ts Outdated
fs.createReadStream(downloadLocation).pipe(unzip.Extract({ path: `${state.dir}/${state.a}`}));
fs.unlink(downloadLocation, async (err) => {
const folderUri = vscode.Uri.file(state.dir);
if (vscode.workspace.workspaceFolders && vscode.workspace.workspaceFolders[0].uri.fsPath != state.dir) {

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.

Is the idea here that if the workspace is empty we automatically add the newly generated project to the current workspace?

@Jonathan-Maciel Jonathan-Maciel Aug 5, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, if it's empty or the target dir is the same as the currrent, then we don't ask to open in a new window.

Comment thread src/liberty/devCommands.ts Outdated
var unzip = require("unzip-stream");
fs.createReadStream(downloadLocation).pipe(unzip.Extract({ path: `${state.dir}/${state.a}`}));
fs.unlink(downloadLocation, async (err) => {
const folderUri = vscode.Uri.file(state.dir);

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.

This seems to open the parent folder of the newly generated project rather than the newly generated project. For example, if I generate into my downloads, my entire downloads folder is opened in VS Code rather than just the app-name project I generated

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in fb3698a. Not sure if it is the flow we are looking for. I also used a timeout to refresh the file explore. I'll go back and use async/await, when I have more time.

Comment thread src/util/gradleUtil.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants