From 74469609040694038e0b820c0bc39b416cddf929 Mon Sep 17 00:00:00 2001 From: tonycheang Date: Fri, 29 Jan 2021 16:49:12 -0800 Subject: [PATCH] Notification for All Feat Pro (#370) * feat: presentational notification handler * improve: migrate hover.js to typescript * rename: kite.more-position -> kite.copilot-docs-from-position * fix: events checks version from package to avoid circular dependencies * refactor: docs-related commands lives with hover provider * refactor: hover.ts -> docs.ts and no default export * add: check for notification on docs command error * improve: render buttons in presentational error notif * clean: remove unused kite.more * improve: use promisifiedKiteAPIRequest insetad of Kite.request Avoids circular dependency when compiling for typescript. * improve: variable naming * improve: log unexpected hover errors * improve: notifyFromError takes optional default error message * fix: don't notify on JSON.parse error It is expected that not all errors will send a notification. * improve: set up ICommandRegistrant[] to signal refactor intent --- .eslintrc.json | 5 +- src/docs.ts | 112 +++++++++++++++++++++++++++++ src/events.js | 4 +- src/hover.js | 50 ------------- src/interfaces.ts | 5 ++ src/kite.js | 58 +++------------ src/metrics.js | 9 +-- src/notifications.js | 74 ++++++++++++------- src/utils.js | 7 ++ test/hover.test.js | 14 ++-- test/json/actions/request_hover.js | 2 +- 11 files changed, 198 insertions(+), 142 deletions(-) create mode 100644 src/docs.ts delete mode 100644 src/hover.js create mode 100644 src/interfaces.ts diff --git a/.eslintrc.json b/.eslintrc.json index 1afe9fa..ea3767e 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -7,7 +7,10 @@ "ecmaVersion": 6, "sourceType": "module" }, - "extends": "eslint:recommended", + "extends": [ + "eslint:recommended", + "@typescript-eslint/no-unused-vars" + ], "env": { "browser": false, "commonjs": true, diff --git a/src/docs.ts b/src/docs.ts new file mode 100644 index 0000000..ca97a84 --- /dev/null +++ b/src/docs.ts @@ -0,0 +1,112 @@ +'use strict'; + +import { + Disposable, + Hover, + MarkdownString, + Position, + TextDocument, + commands, + window, +} from 'vscode'; +import * as KiteAPI from "kite-api"; + +import NotificationsManager from "./notifications"; + +import { ICommandRegistrant } from './interfaces' +import metrics from "./metrics"; +import { hoverPath } from './urls'; +import { + compact, + escapeCommandArguments, + kiteOpen, + promisifiedKiteAPIRequest, +} from './utils'; +import { symbolName, symbolKindMarkdown } from './data-utils'; + +enum Sources { + Command = "Command", + Hover = "Hover", +} + +export class DocsCommands implements ICommandRegistrant { + register(): Disposable[] { + const ret = []; + ret.push(commands.registerCommand("kite.docs-at-cursor", DocsCommands.docsAtCursor)); + ret.push(commands.registerCommand("kite.copilot-docs-from-position", DocsCommands.copilotDocsFromPos)); + return ret; + } + + static async docsAtCursor() { + const editor = window.activeTextEditor; + if (editor) { + const pos = editor.selection.active; + const path = hoverPath(editor.document, pos); + + try { + const resp = await KiteAPI.request({ path }); + if (resp.statusCode === 200) { + commands.executeCommand("kite.copilot-docs-from-position", { + position: pos, + source: Sources.Command + }); + } + } catch(e) { + NotificationsManager.notifyFromError(e); + } + } + } + + static async copilotDocsFromPos(args: { position: Position, source: Sources }): Promise { + metrics.track(`${args.source} See info clicked`); + const doc = window.activeTextEditor.document; + const path = hoverPath(doc, args.position); + try { + const jdata = await promisifiedKiteAPIRequest({ path }); + const data = JSON.parse(jdata); + kiteOpen(`kite://docs/${data.symbol[0].id}`); + } catch(e) { + NotificationsManager.notifyFromError(e); + } + } +} + +export class KiteHoverProvider { + async provideHover(doc: TextDocument, position: Position): Promise { + const path = hoverPath(doc, position); + + try { + const jdata = await promisifiedKiteAPIRequest({ path }); + const data = JSON.parse(jdata); + if (data && data.symbol && data.symbol.length) { + const [symbol] = data.symbol; + + const docsLink = `[Docs](command:kite.copilot-docs-from-position?${escapeCommandArguments({ + position, + source: Sources.Hover, + })})`; + + let defLink: string; + if (data && data.report && data.report.definition && data.report.definition.filename !== '') { + const defData = escapeCommandArguments({ + file: data.report.definition.filename, + line: data.report.definition.line, + source: Sources.Hover, + }); + defLink = `[Def](command:kite.def?${defData})`; + } + + const content = new MarkdownString(`⟠  __${symbolName(symbol).replace('_', '\\_')}__: ${symbolKindMarkdown(symbol)}    ${docsLink}${defLink ? '  ' + defLink : ''}`); + content.isTrusted = true; + + return new Hover(compact([content])); + } + } catch(err) { + // Endpoint can 503 for paywall locked or 404 for not found symbol. Ignore those. + const expected = err.data && (err.data.responseStatus === 503 || err.data.responseStatus === 404) + if (!expected) { + console.log(err, err.data) + } + } + } +} diff --git a/src/events.js b/src/events.js index 3890679..c2b8bed 100644 --- a/src/events.js +++ b/src/events.js @@ -2,8 +2,8 @@ import { version as editor_version } from 'vscode'; -import metrics from './metrics'; -const { version: plugin_version } = metrics; +import kitePkg from '../package.json' +const plugin_version = kitePkg.version import { normalizeDriveLetter } from './urls'; diff --git a/src/hover.js b/src/hover.js deleted file mode 100644 index 84e4538..0000000 --- a/src/hover.js +++ /dev/null @@ -1,50 +0,0 @@ -'use strict'; - -import { MarkdownString, Hover } from 'vscode'; - -import { hoverPath } from './urls'; -import { compact, escapeCommandArguments } from './utils'; -import { symbolName, symbolKindMarkdown } from './data-utils'; - -export default class KiteHoverProvider { - constructor (Kite, isTest) { - this.Kite = Kite; - this.isTest = isTest; - } - - provideHover(doc, position) { - const path = hoverPath(doc, position); - return this.Kite.request({ path }) - .then(data => JSON.parse(data)) - .then(data => { - if (data && data.symbol && data.symbol.length) { - const [symbol] = data.symbol; - - const docsLink = `[Docs](command:kite.more-position?${escapeCommandArguments({ - position, - source: 'Hover', - })})`; - - let defLink; - if (data && data.report && data.report.definition && data.report.definition.filename !== '') { - const defData = escapeCommandArguments({ - file: data.report.definition.filename, - line: data.report.definition.line, - source: 'Hover', - }); - defLink = `[Def](command:kite.def?${defData})`; - } - - const content = new MarkdownString(`⟠  __${symbolName(symbol).replace('_', '\\_')}__: ${symbolKindMarkdown(symbol)}    ${docsLink}${defLink ? '  ' + defLink : ''}`); - content.isTrusted = true; - - const texts = [ - content - ]; - - return new Hover(compact(texts)); - } - }) - .catch(() => {}); - } -} diff --git a/src/interfaces.ts b/src/interfaces.ts new file mode 100644 index 0000000..a1afe29 --- /dev/null +++ b/src/interfaces.ts @@ -0,0 +1,5 @@ +import { Disposable } from 'vscode' + +export interface ICommandRegistrant { + register(): Disposable[] +} diff --git a/src/kite.js b/src/kite.js index db57ad7..081084f 100644 --- a/src/kite.js +++ b/src/kite.js @@ -16,7 +16,7 @@ import { PythonSignaturesSupport, IsSupportedFile, } from "./constants"; -import KiteHoverProvider from "./hover"; +import { KiteHoverProvider, DocsCommands } from "./docs"; import KiteCompletionProvider from "./completion"; import KiteSignatureProvider from "./signature"; import KiteDefinitionProvider from "./definition"; @@ -25,7 +25,7 @@ import EditorEvents from "./events"; import NotificationsManager from "./notifications"; import localconfig from "./localconfig"; import metrics from "./metrics"; -import { statusPath, hoverPath } from "./urls"; +import { statusPath } from "./urls"; import Rollbar from "rollbar"; import { editorsForDocument, @@ -91,7 +91,7 @@ export const Kite = { this.disposables.push( vscode.languages.registerHoverProvider( PythonHoverSupport(), - new KiteHoverProvider(Kite) + new KiteHoverProvider() ) ); this.disposables.push( @@ -195,6 +195,10 @@ export const Kite = { this.disposables.push(this.statusBarItem); + // ICommandRegistrant[] (target TS refactor for commands below) + const commandRegistrants = [new DocsCommands()]; + commandRegistrants.forEach(cmdreg => this.disposables.push(...cmdreg.register())); + this.disposables.push( vscode.commands.registerCommand("kite.insert-completion", ({ lang, completion }) => { metrics.increment(`vscode_kite_${lang}_completions_inserted`); @@ -219,7 +223,7 @@ export const Kite = { vscode.commands.registerTextEditorCommand("kite.related-code-from-file", (textEditor) => { KiteAPI .requestRelatedCode("vscode", vscode.env.appRoot, textEditor.document.fileName) - .catch(NotificationsManager.getRelatedCodeErrHandler()); + .catch(err => NotificationsManager.notifyFromError(err, "Oops! Something went wrong with Code Finder. Please try again later.")); }) ); @@ -229,7 +233,7 @@ export const Kite = { const oneBasedLineNo = zeroBasedLineNo+1; KiteAPI .requestRelatedCode("vscode", vscode.env.appRoot, textEditor.document.fileName, oneBasedLineNo) - .catch(NotificationsManager.getRelatedCodeErrHandler()); + .catch(err => NotificationsManager.notifyFromError(err, "Oops! Something went wrong with Code Finder. Please try again later.")); }) ); @@ -241,29 +245,6 @@ export const Kite = { }) ); - this.disposables.push( - vscode.commands.registerCommand("kite.more", ({ id, source }) => { - metrics.track(`${source} See info clicked`); - kiteOpen(`kite://docs/${id}`); - }) - ); - - this.disposables.push( - vscode.commands.registerCommand( - "kite.more-position", - ({ position, source }) => { - metrics.track(`${source} See info clicked`); - const doc = vscode.window.activeTextEditor.document; - const path = hoverPath(doc, position); - return this.request({ path }) - .then(data => JSON.parse(data)) - .then(data => { - kiteOpen(`kite://docs/${data.symbol[0].id}`); - }); - } - ) - ); - this.disposables.push( vscode.commands.registerCommand("kite.web-url", url => { open(url.replace(/;/g, "%3B")); @@ -341,27 +322,6 @@ export const Kite = { }) ); - this.disposables.push( - vscode.commands.registerCommand("kite.docs-at-cursor", () => { - const editor = vscode.window.activeTextEditor; - - if (editor) { - const pos = editor.selection.active; - const { document } = editor; - - const path = hoverPath(document, pos); - KiteAPI.request({ path }).then(resp => { - if (resp.statusCode === 200) { - vscode.commands.executeCommand("kite.more-position", { - position: pos, - source: "Command" - }); - } - }); - } - }) - ); - this.disposables.push( vscode.commands.registerCommand( "kite.usage", diff --git a/src/metrics.js b/src/metrics.js index 6f2b729..73dd117 100644 --- a/src/metrics.js +++ b/src/metrics.js @@ -6,9 +6,9 @@ import crypto from 'crypto'; import mixpanel from 'mixpanel'; import Logger from "kite-connector/lib/logger"; -import kitePkg from "../package.json"; import localconfig from "./localconfig.js"; import { metricsCounterPath, metricsCompletionSelectedPath } from "./urls"; +import { promisifiedKiteAPIRequest } from './utils' const OS_VERSION = os.type() + " " + os.release(); @@ -16,8 +16,6 @@ const EDITOR_UUID = vscode.env.machineId; const MIXPANEL_TOKEN = "fb6b9b336122a8b29c60f4c28dab6d03"; -import { Kite } from './kite'; - const mpClient = mixpanel.init(MIXPANEL_TOKEN, { protocol: "https", }); @@ -40,7 +38,7 @@ function sendCompletionSelected(lang, completion) { const path = metricsCompletionSelectedPath(); - return Kite.request( + return promisifiedKiteAPIRequest( { path, method: "POST" @@ -62,7 +60,7 @@ function sendFeatureMetric(name) { Logger.debug("feature metric:", name); - return Kite.request( + return promisifiedKiteAPIRequest( { path, method: "POST" @@ -104,7 +102,6 @@ export default { increment: name => sendFeatureMetric(name), getOsName, sendCompletionSelected, - version: kitePkg.version, track: (event, props) => { if (process.env.NODE_ENV !== "production") { console.log(`tracking ${event}`, props); diff --git a/src/notifications.js b/src/notifications.js index fd822d3..14b5529 100644 --- a/src/notifications.js +++ b/src/notifications.js @@ -22,38 +22,60 @@ export default class NotificationsManager { } } - static getRelatedCodeErrHandler() { - return (err) => { - if (!err) { - return; - } - const showDefaultErrMsg = () => vscode.window.showWarningMessage( - "Oops! Something went wrong with Code Finder. Please try again later." - ); - if (!err.data) { - showDefaultErrMsg(); - return; + // notifyFromError takes an error from a request and parses it + // If it matches the expected presentational API, it will notify + // using the message. Otherwise it notifies using the defaultMessage + // if it's passed one. + static async notifyFromError(err, defaultMessage) { + function tryNotifyDefault() { + if (defaultMessage) { + vscode.window.showWarningMessage(defaultMessage) } + } + if (!err.data) { + tryNotifyDefault() + return + } - const { state, responseData } = err.data; + const { state, responseData } = err.data; + if (!responseData) { + tryNotifyDefault() + return + } - if (state && state <= KiteAPI.STATES.UNREACHABLE) { - vscode.window.showWarningMessage("Kite could not be reached. Please check that Kite engine is running."); - return; - } + if (state && state <= KiteAPI.STATES.UNREACHABLE) { + vscode.window.showWarningMessage("Kite could not be reached. Please check that Kite engine is running."); + return; + } - try { - const { message } = JSON.parse(responseData); - if (message && typeof responseData === 'string') { - vscode.window.showWarningMessage(message); - return; + try { + const { notification: notif, message } = JSON.parse(responseData); + if (notif) { + // Since warning messages don't have a title, join it with body + let title = notif.title; + if (title !== "" && !title.endsWith('.')) { + title += "."; } - } catch (e) { - console.error(e); + const buttonsText = notif.buttons.map(button => button.text) + vscode.window + .showWarningMessage([title, notif.body].join(" "), ...buttonsText) + .then(selectedText => { + const selectedButton = notif.buttons.find(button => button.text == selectedText); + switch(selectedButton.action) { + case "open": + open(selectedButton.link) + case "dismiss": + // no-op closes + } + }) + } else if (message) { + vscode.window.showWarningMessage(message); + } else { + tryNotifyDefault(); } - - showDefaultErrMsg(); - }; + } catch { + tryNotifyDefault(); + } } static showWelcomeNotification(config, openKiteTutorial) { diff --git a/src/utils.js b/src/utils.js index 876df66..7e7a755 100644 --- a/src/utils.js +++ b/src/utils.js @@ -4,6 +4,12 @@ import vscode from 'vscode'; import cp from 'child_process'; import os from 'os'; +import KiteAPI from "kite-api"; + +function promisifiedKiteAPIRequest(req, data) { + return KiteAPI.request(req, data).then(resp => promisifyReadResponse(resp)); +}; + const compact = a => a.filter(v => v && v !== ''); const uniq = a => a.reduce((m, v) => m.indexOf(v) === -1 ? m.concat(v) : m, []); @@ -202,6 +208,7 @@ export { promisifyReadResponse, promisifyRequest, secondsSince, + promisifiedKiteAPIRequest, stopPropagationAndDefault, truncate, uniq, diff --git a/test/hover.test.js b/test/hover.test.js index 0429b56..bbb5006 100644 --- a/test/hover.test.js +++ b/test/hover.test.js @@ -5,14 +5,14 @@ import { assert } from 'chai'; import { withKite, withKiteRoutes } from 'kite-api/test/helpers/kite'; import { fakeResponse } from 'kite-api/test/helpers/http'; -import { fixtureURI, Kite } from './helpers'; -import KiteHoverProvider from '../src/hover'; +import { fixtureURI } from './helpers'; +import { KiteHoverProvider } from '../src/docs'; describe('KiteHoverProvider', () => { let provider; beforeEach(() => { - provider = new KiteHoverProvider(Kite, true); + provider = new KiteHoverProvider(); }); withKite({ reachable: true }, () => { describe('for a python function with a definition', () => { @@ -32,7 +32,7 @@ describe('KiteHoverProvider', () => { assert.equal(contents.length, 1); const contentString = contents[0].value; - assert.include(contentString, '[Docs](command:kite.more-position?{"position":{"line":19,"character":13},"source":"Hover"}'); + assert.include(contentString, '[Docs](command:kite.copilot-docs-from-position?{"position":{"line":19,"character":13},"source":"Hover"}'); assert.include(contentString, '[Def](command:kite.def?{"file":"sample.py","line":50,"source":"Hover"})'); }); }); @@ -55,7 +55,7 @@ describe('KiteHoverProvider', () => { assert.equal(contents.length, 1); const contentString = contents[0].value; - assert.include(contentString, '[Docs](command:kite.more-position?{"position":{"line":19,"character":13},"source":"Hover"}'); + assert.include(contentString, '[Docs](command:kite.copilot-docs-from-position?{"position":{"line":19,"character":13},"source":"Hover"}'); }); }); }); @@ -78,7 +78,7 @@ describe('KiteHoverProvider', () => { assert.equal(contents.length, 1); const contentString = contents[0].value; - assert.include(contentString, '[Docs](command:kite.more-position?{"position":{"line":19,"character":13},"source":"Hover"}'); + assert.include(contentString, '[Docs](command:kite.copilot-docs-from-position?{"position":{"line":19,"character":13},"source":"Hover"}'); const data = JSON.parse(osjson); data["symbol"][0]["value"].forEach(({ type }) => { @@ -106,7 +106,7 @@ describe('KiteHoverProvider', () => { assert.equal(contents.length, 1); const contentString = contents[0].value; - assert.include(contentString, "[Docs](command:kite.more-position"); + assert.include(contentString, "[Docs](command:kite.copilot-docs-from-position"); assert.include(contentString, '"position":{"line":19,"character":13}'); const data = JSON.parse(selfjson); diff --git a/test/json/actions/request_hover.js b/test/json/actions/request_hover.js index 083f283..98fcdd5 100644 --- a/test/json/actions/request_hover.js +++ b/test/json/actions/request_hover.js @@ -2,7 +2,7 @@ const vscode = require('vscode'); const {kite: Kite} = require('../../../src/kite'); -const KiteHoverProvider = require('../../../src/hover'); +const { KiteHoverProvider } = require('../../../src/docs'); module.exports = () => { beforeEach('requesting hover', () => {