-
Notifications
You must be signed in to change notification settings - Fork 9
fix: yeoman-environment version update + code refactor + tests #52
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
Conversation
backend/src/contributors.ts
Outdated
const contributorId = _.get(uiOptions, "contributorId"); | ||
const snippetName = _.get(uiOptions, "snippetName"); | ||
if (contributorId && snippetName) { | ||
const api = await Contributors.apiMap.get(contributorId); |
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.
feel to me like there is no await for the activate() method.
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.
I would like to keep a getApi method.
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.
extension.activate() returns Thenable, so it is OK to use await
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.
I have added a new test with activate method
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.
const apiPromise = Contributors.apiMap.get(contributorId);
const api = await
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.
Done
Contributors.getSnippet(_.get(uiOptions, "snippet", uiOptions)).then(snippet => { | ||
if (_.isNil(snippet)) { | ||
this.webViewPanel.dispose(); | ||
return vscode.window.showErrorMessage("Can not find snippet."); |
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.
I consider remove the vscode notification. and only only log it.
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.
We talked to leave it, didn't we ?
this.initWebviewPanel(); | ||
Contributors.getSnippet(_.get(uiOptions, "snippet", uiOptions)).then(snippet => { | ||
if (_.isNil(snippet)) { | ||
this.webViewPanel.dispose(); |
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.
Nice 💯
backend/src/contributors.ts
Outdated
const contributorId = _.get(uiOptions, "contributorId"); | ||
const snippetName = _.get(uiOptions, "snippetName"); | ||
if (contributorId && snippetName) { | ||
const api = await Contributors.apiMap.get(contributorId); |
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.
const apiPromise = Contributors.apiMap.get(contributorId);
const api = await
} | ||
} | ||
private static getApiPromise(extension: vscode.Extension<any>) { | ||
return (extension.isActive ? extension.exports : extension.activate()); |
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.
extension.exports is not a promise, but we can keep the name.
as we just discuss. lets activate (if not already active) only when trying to get the snippet, and then update the apiMap. |
backend/src/contributors.ts
Outdated
const extensionName: string = _.get(extension, "packageJSON.name"); | ||
const extensionPublisher: string = _.get(extension, "packageJSON.publisher"); | ||
const extensionId = `${extensionPublisher}.${extensionName}`; |
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.
private static getExtensionId(extension: vscode.Extension)
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.
done
backend/src/contributors.ts
Outdated
} | ||
} | ||
|
||
private static getApiPromise(extension: vscode.Extension<any>) { | ||
return (extension.isActive ? extension.exports : extension.activate()); | ||
return (extension.isActive ? Promise.resolve(extension.exports) : extension.activate()); |
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.
no need to wrap extension.exports with Promise.
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.
done
const contributorId = _.get(contributerInfo, "contributorId"); | ||
const snippetName = _.get(contributerInfo, "snippetName"); | ||
const extension = Contributors.getContributorExtension(contributorId); | ||
if (extension) { |
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 extension not found?
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.
in this case tab is closed and we show error message
backend/src/contributors.ts
Outdated
if (!extension.isActive) { | ||
public static async getSnippet(contributerInfo: any) { | ||
const contributorId = _.get(contributerInfo, "contributorId"); | ||
const snippetName = _.get(contributerInfo, "snippetName"); |
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.
move into the try block
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.
done
No description provided.