Skip to content
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

[Toolchain] Uninstall toolchain #707

Merged
merged 1 commit into from
May 25, 2022
Merged

[Toolchain] Uninstall toolchain #707

merged 1 commit into from
May 25, 2022

Conversation

jyoungyun
Copy link
Collaborator

This commit uninstalls the installed toolchain.
After uninstalled the toolchain, the toolchain view will be updated
automatically.

ONE-vscode-DCO-1.0-Signed-off-by: Jiyoung Yun [email protected]

@jyoungyun

This comment was marked as outdated.

@jyoungyun jyoungyun mentioned this pull request May 24, 2022
57 tasks
@@ -28,14 +28,15 @@ export class ToolchainNode extends vscode.TreeItem {
public readonly label: string,
public readonly collapsibleState: vscode.TreeItemCollapsibleState,
public readonly type: NodeType,
public readonly backend?: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Q. Any reason to assign backend as string | undefined? If you assign it as just the only string, we don't have to care about backend is undefined in ts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The backend is optional parameter so it can either have a value based on the type defined or its value can be undefined. However, when I use backend: string | undefined code, this value must be inputted. But NodeType.backend does not use backend string. So I use it as optional parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your point.

This commit uninstalls the installed toolchain.
After uninstalled the toolchain, the toolchain view will be updated
automatically.

ONE-vscode-DCO-1.0-Signed-off-by: Jiyoung Yun <[email protected]>
@jyoungyun jyoungyun requested a review from a team May 25, 2022 02:14
@jyoungyun
Copy link
Collaborator Author

PTAL /cc @Samsung/one-vscode

Copy link
Contributor

@YongseopKim YongseopKim left a comment

Choose a reason for hiding this comment

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

LGTM!

IHMO, items in the future

  • ToolchainNode seems to be divided to something for backend and something for toolchain

@llFreetimell llFreetimell merged commit 89b9a28 into Samsung:main May 25, 2022
@jyoungyun jyoungyun deleted the view/toolchain/support_uninstall branch May 26, 2022 00:30
@jyoungyun
Copy link
Collaborator Author

jyoungyun commented May 26, 2022

  • ToolchainNode seems to be divided to something for backend and something for toolchain

Um, ToolchainNode can't be separated for backend and toolchain. Because these item should be shared in this tree view. So it has the same root interface to share these one. I can separate the 3 class for parent and 2 children for backend and toolchain that inherit parent class. However, in most tree view example codes, it is designed to implement both parent and children nodes as one node. So I'm not sure if it really needs to be modified.... Anyway, I will consider about your suggestion more. :)

@YongseopKim
Copy link
Contributor

Trial: #717

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.

4 participants