Add "Show log" button for all messages#274
Conversation
This change ensures that "Show log" is available on all messages from the extension. It's important to note that the only place that was specifying an "item" before was doing it incorrectly. That's been fixed. Closes github#287
|
Added the changelog update and this includes adding a few lines for changes in previous PRs that I forgot to add. |
jcreedcmu
left a comment
jcreedcmu
left a comment
There was a problem hiding this comment.
Do you see a convenient way to have
const response = await Window.showInformationMessage('CodeQL Query Server restarted.', 'Show Log');
in extension.ts also use this helper? Slightly awkward since it's a different logger object... Leave it up to you.
Capitalization of Log/log should be made consistent anyhow. Either one's one.
| if (response === 'Show Log') { | ||
| qs.showLog(); | ||
| } | ||
| helpers.showAndLogInformationMessage('CodeQL Query Server restarted.'); |
There was a problem hiding this comment.
Sorry, my fault for not being clear; the helper just needs an extra (optional? defaulting to logging.logger?) OutputChannelArgument argument or something, since the log stream that this existing 'Show Log' button shows (which I think is what we want) is the one named 'CodeQL Query Server', rather than the default 'CodeQL Extension Log', which is what helpers.ts is getting from logging.logger.
There was a problem hiding this comment.
Darn. You're right. I wasn't reading carefully enough. I'll fix. Though, it's a little more complex since the last argument is varargs, but I should be able to do something.
There was a problem hiding this comment.
Seeing as we're making the wrappers around the underlying varargs methods on vscode.window already, I wouldn't mind at all refactoring to make them take an explicit array argument instead.
There was a problem hiding this comment.
What about if it takes a message arg and an options arg. This will make it more flexible and future-proof since I'm sure we'll want to augment with different behaviour later.
| async function internalShowAndLog(message: string, fn: Function, ...items: string[]): Promise<string | undefined> { | ||
| logger.log(message); | ||
| const label = 'Show log'; | ||
| async function internalShowAndLog(message: string, items: string[], outputLogger = logger, fn: Function): Promise<string | undefined> { |
There was a problem hiding this comment.
Totally a nit, but I think I'd prefer the specific type (message: string, label: string, ...items: string[]) => Thenable<string | undefined> to just Function.
Also, changes the showAndLog* signatures to accept an optional logger argument.
|
Also, Closes #273 |
2 participants
This change ensures that "Show log" is available on all messages
from the extension. It's important to note that the only place that
was specifying an "item" before was doing it incorrectly. That's
been fixed.
Closes #287
Checklist