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.

Sign up for GitHub

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

Jump to bottom

[Toolchain] Revisit ToolchainProvider class #717

Merged
merged 1 commit into from
Jul 11, 2022
Merged

[Toolchain] Revisit ToolchainProvider class #717

merged 1 commit into from
Jul 11, 2022

Conversation

YongseopKim
Copy link
Contributor

@YongseopKim YongseopKim commented May 26, 2022 •
edited
Loading

This commit revisits ToolchainProver class in src/Toolchain.

@YongseopKim YongseopKim added DRAFT Something like playground NO MERGE labels May 26, 2022
@YongseopKim YongseopKim requested a review from jyoungyun May 26, 2022 05:41
@YongseopKim YongseopKim mentioned this pull request May 26, 2022
Merged
@YongseopKim YongseopKim force-pushed the draft/refactor_ToolchainProvider branch from 3585cf1 to 5a3a6ad Compare July 5, 2022 09:05
@YongseopKim YongseopKim changed the title [Draft] Refactor toolchain provider [Toolchain] Revisit ToolchainProver class Jul 5, 2022
@YongseopKim YongseopKim removed DRAFT Something like playground NO MERGE labels Jul 5, 2022
@YongseopKim YongseopKim marked this pull request as ready for review July 5, 2022 09:06
@YongseopKim
Copy link
Contributor Author

The points

  • ToolchainProvider can be used as TreeDataProvider as possible
  • To do it, let's introduce Node classes(BaseNode <- BackendNode or ToolchainNode)
  • BackendNode is an expression for Backend {} in ToolchainView
  • ToolchainNode is an expression for Toolchain o in ToolchainView
  • Let's introduce NodeBuilder btw Node and Provider

@YongseopKim
Copy link
Contributor Author

PTAL @jyoungyun

YongseopKim
YongseopKim commented Jul 5, 2022
View reviewed changes
src/Tests/Backend/Backend.test.ts Outdated
assert.deepStrictEqual(value, backend);
}
});
});

teardown(function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part can be separated pr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#956

YongseopKim
YongseopKim commented Jul 5, 2022
View reviewed changes
src/Tests/Toolchain/ToolchainEnv.test.ts Outdated

// TODO: Move this to MockCompiler.ts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#957

YongseopKim
YongseopKim commented Jul 5, 2022
View reviewed changes
src/Toolchain/ToolchainProvider.ts Outdated
@@ -17,88 +17,105 @@
import * as vscode from 'vscode';

import {Toolchain} from '../Backend/Toolchain';
import {DebianToolchain} from '../Backend/ToolchainImpl/DebianToolchain';
import {Job, JobCallback} from '../Project/Job';
import {Job} from '../Project/Job';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better to read this file as one file not diff.

YongseopKim
YongseopKim commented Jul 5, 2022
View reviewed changes
src/Tests/Toolchain/ToolchainProvider.test.ts
import {BackendNode, BaseNode, NodeBuilder, ToolchainNode, ToolchainProvider} from '../../Toolchain/ToolchainProvider';


// TODO: Move this to MockCompiler.ts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#957

@YongseopKim YongseopKim requested a review from a team July 6, 2022 02:33
jyoungyun
jyoungyun previously approved these changes Jul 6, 2022
View reviewed changes
Copy link
Collaborator

@jyoungyun jyoungyun left a comment

Choose a reason for hiding this comment

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

LGTM

@YongseopKim YongseopKim requested a review from a team July 6, 2022 23:54
@YongseopKim
Copy link
Contributor Author

PTAL @Samsung/one-vscode

@YongseopKim YongseopKim force-pushed the draft/refactor_ToolchainProvider branch from 5a3a6ad to 94459df Compare July 8, 2022 02:45
@YongseopKim
Copy link
Contributor Author

Rebased. PTAL @jyoungyun and @Samsung/one-vscode

jyoungyun
jyoungyun previously approved these changes Jul 8, 2022
View reviewed changes
Copy link
Collaborator

@jyoungyun jyoungyun left a comment

Choose a reason for hiding this comment

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

LGTM

@YongseopKim
[Toolchain] Revisit ToolchainProver class
8c0e8c6 
This commit revisits ToolchainProver class in src/Toolchain.

ONE-vscode-DCO-1.0-Signed-off-by: Yongseop Kim <[email protected]>
@YongseopKim YongseopKim force-pushed the draft/refactor_ToolchainProvider branch from 94459df to 8c0e8c6 Compare July 8, 2022 07:40
@YongseopKim
Copy link
Contributor Author

Rebased again... PTAL @jyoungyun and @Samsung/one-vscode

@YongseopKim YongseopKim requested review from a team July 8, 2022 07:51
jyoungyun
jyoungyun approved these changes Jul 8, 2022
View reviewed changes
Copy link
Collaborator

@jyoungyun jyoungyun left a comment

Choose a reason for hiding this comment

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

LGTM

Please review this PR. /cc @Samsung/one-vscode

@jyoungyun
Copy link
Collaborator

/cc @YongseopKim PR title has typo. ToolchainProver => ToolchainProvider

@YongseopKim YongseopKim changed the title [Toolchain] Revisit ToolchainProver class [Toolchain] Revisit ToolchainProvider class Jul 11, 2022
llFreetimell
llFreetimell approved these changes Jul 11, 2022
View reviewed changes
Copy link
Contributor

@llFreetimell llFreetimell left a comment

Choose a reason for hiding this comment

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

LGTM

@llFreetimell llFreetimell merged commit 28bd411 into Samsung:main Jul 11, 2022
@YongseopKim YongseopKim deleted the draft/refactor_ToolchainProvider branch July 11, 2022 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyoungyun jyoungyun jyoungyun approved these changes

@llFreetimell llFreetimell llFreetimell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@YongseopKim @jyoungyun @llFreetimell