Don't cache failed getMesonTasks() result. #141
Conversation
|
Could you explain the problem this fixes? |
| @@ -68,11 +68,15 @@ export async function activate(ctx: vscode.ExtensionContext) { | |||
| controller.createRunProfile("Meson run test", vscode.TestRunProfileKind.Run, (request, token) => testRunHandler(controller, request, token), true) | |||
| ctx.subscriptions.push(controller); | |||
|
|
|||
| let mesonTasks: Thenable<vscode.Task[]> | null = null; | |||
| let mesonTasks: Promise<vscode.Task[]> | null = null; | |||
There was a problem hiding this comment.
There's no catch() on a Thenable.
|
@lukester1975 if it fails, what hope so you have that it will succeed next time? I think only a reconfigure has a chance to fix the error potentially. |
|
Yeah, this is mainly about UX. Caching a failed But also reasoning about whether Thanks |
|
@xclaesse should we just merge this and spin a release? |
|
@tristan957 TBH I was not convinced about this PR, if getting tasks failed once, it won't magically succeed next time. The only thing this could achieve is slowing down everything (load tasks is slow) and spamming user with the error message. Unless I missed something? AFAIK, the only chance we have to have loading tasks succeed again is after a reconfigure, in that case we do that already. |
|
Maybe I was unclear so let me try again: it's not about magically succeeding next time. It's about the lack of any feedback running tasks after caching the failure. Not caching the failed result was the smallest mod I could see to achieve this. Such feedback is not spam... spam is unsolicited. Worrying about "slowing down everything" isn't an issue for the super rare case of |
|
Seems pretty compelling to me after that explanation. |
|
I should give it a try to see how it goes in practice. I'm in phone ATM, can't test until Monday. The reason I'm prudent here is because when I implemented that cache it was because vscode calls that method a lot to populate menus, and parsing interpretation JSON files is super slow. So I would like to be sure it shows error only when user really does an action and not spuriously just because vscode refresh a menu. Maybe it should cache the error state to not retry parsing and show error immediately? |
I don't think it should rely on
changeHandlerbeing invoked to clear the cache.