Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upfix: #593 by adding global flag to remove cache #602
Conversation
public async meetRequirements(context: ExtensionContext): Promise<boolean> { | ||
const isUserFresh: boolean | undefined = context.globalState.get(globalStateLeetcodeIsUserFresh); | ||
if (isUserFresh !== false) { | ||
await this.removeOldCache(); |
This comment has been minimized.
This comment has been minimized.
jdneo
Jul 22, 2020
Member
Reuse the command we have?
https://github.com/LeetCode-OpenSource/vscode-leetcode/blob/master/src/extension.ts#L51
This comment has been minimized.
This comment has been minimized.
yihong0618
Jul 22, 2020
Author
Contributor
Seems a little different.The command way use leetcode-cli way not delete the whole dir, let me check if it works fine too.
This comment has been minimized.
This comment has been minimized.
yihong0618
Jul 22, 2020
Author
Contributor
@jdneo
delete cache only delete the user session but not the plugin.js cache, still have that issue. So I think we can not use it..
This comment has been minimized.
This comment has been minimized.
jdneo
Jul 22, 2020
•
Member
Maybe we need to change the implementation of the command delete cache
. Considering that the current implementation does not clean entirely?
This comment has been minimized.
This comment has been minimized.
yihong0618
Jul 22, 2020
Author
Contributor
Yes, current one only clean the user info(include session), but we change the delete cache
also have one problem beacuse user may have two account one leetcode one leetcode-cn, if we change that. both will clean, but user may only want to clean the current cache.
This comment has been minimized.
This comment has been minimized.
jdneo
Jul 22, 2020
Member
I see. So let's keep the implementation. Maybe later we can provide two option in this command. Something like Session
and All
. Anyway it's not a high priority
public async meetRequirements(): Promise<boolean> { | ||
public async meetRequirements(context: ExtensionContext): Promise<boolean> { | ||
const isUserFresh: boolean | undefined = context.globalState.get(globalStateLeetcodeIsUserFresh); | ||
if (isUserFresh !== false) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
yihong0618
Jul 22, 2020
Author
Contributor
if (!isUserFresh)
@jdneo
Seems not the same logic if use if (!isUserFresh)
await this.executeCommandEx(this.nodeExecutable, [await this.getLeetCodeBinaryPath(), "plugin", "-i", plugin]); | ||
} | ||
} | ||
context.globalState.update(globalStateLeetcodeIsUserFresh, false); |
This comment has been minimized.
This comment has been minimized.
yihong0618
Jul 22, 2020
Author
Contributor
In the end of activate() I set it to false, in the first it should be undefined so I use (isUserFresh !== false)
logic to check.
This comment has been minimized.
This comment has been minimized.
jdneo
Jul 22, 2020
Member
Then instead of calling it isUserFresh
, it would be better to call it HasInited
IMO. And set it to true
at the end of activate()
The aim of that is to avoid the two falsy value undefined
and false
has two different meanings.
This comment has been minimized.
This comment has been minimized.
@@ -84,7 +85,7 @@ class LeetCodeExecutor implements Disposable { | |||
} | |||
|
|||
public async signOut(): Promise<string> { | |||
return await await this.executeCommandEx(this.nodeExecutable, [await this.getLeetCodeBinaryPath(), "user", "-L"]); |
This comment has been minimized.
This comment has been minimized.
yihong0618
Jul 22, 2020
Author
Contributor
It seems that its an old typo so I changed it too, am I right?
This comment has been minimized.
This comment has been minimized.
@@ -114,3 +114,5 @@ export enum DescriptionConfiguration { | |||
Both = "Both", | |||
None = "None", | |||
} | |||
|
|||
export const globalStateLeetcodeHasInited: string = "leetcode.isUserFresh"; |
This comment has been minimized.
This comment has been minimized.
jdneo
Jul 23, 2020
•
Member
Please also change the value. No need to have the prefix globalState
I think. Since when you calling the API, that API name already tells it's a global state
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Thanks @yihong0618 ! |
yihong0618 commentedJul 22, 2020
No description provided.