From 0d88c95b41891d4a9a3e2ef105dfc33cdbeff523 Mon Sep 17 00:00:00 2001 From: Matt Seddon <37993418+mattseddon@users.noreply.github.com> Date: Fri, 3 Feb 2023 05:52:00 +1100 Subject: [PATCH] Remove read file contents request sent to extension by language server (use is file check) (#3198) --- extension/src/fileSystem/index.ts | 23 +++++--------- extension/src/languageClient/index.ts | 13 ++++++-- .../src/test/suite/fileSystem/index.test.ts | 31 ++++++++++++------- languageServer/src/languageServer.ts | 18 +++++------ languageServer/src/test/definitions.test.ts | 7 ++--- .../src/test/utils/setup-test-connections.ts | 2 +- languageServer/src/textDocument.ts | 16 +++------- 7 files changed, 53 insertions(+), 57 deletions(-) diff --git a/extension/src/fileSystem/index.ts b/extension/src/fileSystem/index.ts index fd611afdc1..ce9da60515 100644 --- a/extension/src/fileSystem/index.ts +++ b/extension/src/fileSystem/index.ts @@ -22,14 +22,19 @@ import { delay } from '../util/time' export const exists = (path: string): boolean => existsSync(path) -export const isDirectory = (path: string): boolean => { +const checkStats = (path: string, check: 'isDirectory' | 'isFile'): boolean => { try { - return lstatSync(path).isDirectory() + return lstatSync(path)[check]() } catch { return false } } +export const isDirectory = (path: string): boolean => + checkStats(path, 'isDirectory') + +export const isFile = (path: string): boolean => checkStats(path, 'isFile') + export const getModifiedTime = (path: string): number => lstatSync(path).mtime.getTime() @@ -75,7 +80,7 @@ export const findAbsoluteDvcRootPath = async ( const findDotGitDir = (gitRoot: string) => { const dotGitPath = join(gitRoot, gitPath.DOT_GIT) - const isSubmodule = lstatSync(dotGitPath).isFile() + const isSubmodule = isFile(dotGitPath) if (isSubmodule) { const dotGitAsFileContent = readFileSync(dotGitPath, 'utf8') const gitDirPrefix = 'gitdir: ' @@ -204,15 +209,3 @@ export const getBinDisplayText = ( ? '.' + sep + relative(workspaceRoot, path) : path } - -export const readFileContents = ( - uriString: string -): { contents: string } | null => { - try { - const uri = Uri.parse(uriString) - if (!isDirectory(uri.fsPath)) { - return { contents: readFileSync(uri.fsPath, 'utf8') } - } - } catch {} - return null -} diff --git a/extension/src/languageClient/index.ts b/extension/src/languageClient/index.ts index ea36cd9cd6..b0968e7093 100644 --- a/extension/src/languageClient/index.ts +++ b/extension/src/languageClient/index.ts @@ -1,4 +1,4 @@ -import { workspace } from 'vscode' +import { Uri, workspace } from 'vscode' import { LanguageClient as Client, LanguageClientOptions, @@ -7,7 +7,7 @@ import { } from 'vscode-languageclient/node' import { documentSelector, serverModule } from 'dvc-vscode-lsp' import { Disposable } from '../class/dispose' -import { readFileContents } from '../fileSystem' +import { isFile } from '../fileSystem' export class LanguageClient extends Disposable { private client: Client @@ -32,7 +32,14 @@ export class LanguageClient extends Disposable { ) this.dispose.track( - this.client.onRequest('readFileContents', readFileContents) + this.client.onRequest('isFile', (uri: string) => { + try { + const path = Uri.parse(uri).fsPath + return { isFile: isFile(path) } + } catch { + return null + } + }) ) void this.client.start() diff --git a/extension/src/test/suite/fileSystem/index.test.ts b/extension/src/test/suite/fileSystem/index.test.ts index 5243d9a040..5179984ace 100644 --- a/extension/src/test/suite/fileSystem/index.test.ts +++ b/extension/src/test/suite/fileSystem/index.test.ts @@ -1,6 +1,5 @@ import { join, resolve } from 'path' import process from 'process' -import { Uri } from 'vscode' import { afterEach, beforeEach, describe, it, suite } from 'mocha' import { expect } from 'chai' import { Disposable } from '@hediet/std/disposable' @@ -11,7 +10,8 @@ import { checkSignalFile, exists, getGitPath, - readFileContents + isDirectory, + isFile } from '../../../fileSystem' import { gitPath } from '../../../cli/git/constants' import { GitReader } from '../../../cli/git/reader' @@ -100,19 +100,28 @@ suite('File System Test Suite', () => { }) }) - describe('readFileContents', () => { - it('should read the contents of a file when it exists', () => { - const uriString = Uri.file(join(dvcDemoPath, 'train.py')).toString() + describe('isFile', () => { + it('should return true when the path is a file', () => { + const path = standardizePath(join(dvcDemoPath, 'train.py')) - const file = readFileContents(uriString) - expect(file?.contents).to.contain('main()') + const result = isFile(path) + expect(result).to.be.true }) - it('should return null when the file cannot be found', () => { - const uriString = 'file:///some/fun/file.txt' + it('should return false when the file cannot be found', () => { + const path = standardizePath(join('some', 'fun', 'file.txt')) - const contents = readFileContents(uriString) - expect(contents).to.be.null + const result = isFile(path) + expect(result).to.be.false + }) + + it('should return false for a directory', () => { + const path = standardizePath(join(dvcDemoPath, 'training')) + + expect(isDirectory(path)).to.be.true + + const result = isFile(path) + expect(result).to.be.false }) }) }) diff --git a/languageServer/src/languageServer.ts b/languageServer/src/languageServer.ts index f91c26b014..16b20de401 100644 --- a/languageServer/src/languageServer.ts +++ b/languageServer/src/languageServer.ts @@ -15,11 +15,7 @@ import { } from 'vscode-languageserver/node' import { TextDocument } from 'vscode-languageserver-textdocument' import { URI } from 'vscode-uri' -import { - getTextDocumentLocation, - getUriLocation, - symbolAt -} from './textDocument' +import { getLocationToOpen, symbolAt } from './textDocument' export class LanguageServer { private documentsKnownToEditor!: TextDocuments @@ -58,7 +54,7 @@ export class LanguageServer { .all() .filter(doc => URI.parse(doc.uri).fsPath.endsWith(filePath)) - return matchingFiles.map(doc => getTextDocumentLocation(doc)) + return matchingFiles.map(doc => getLocationToOpen(doc.uri)) } private async onDefinition(params: DefinitionParams, connection: Connection) { @@ -98,11 +94,11 @@ export class LanguageServer { const possiblePath = URI.parse( [dirname(document.uri), possibleFile].join('/') ).toString() - const file = await connection.sendRequest<{ - contents: string - } | null>('readFileContents', possiblePath) - if (file) { - const location = getUriLocation(possiblePath, file.contents) + const result = await connection.sendRequest<{ + isFile: boolean + } | null>('isFile', possiblePath) + if (result?.isFile) { + const location = getLocationToOpen(possiblePath) fileLocations.push(location) } } diff --git a/languageServer/src/test/definitions.test.ts b/languageServer/src/test/definitions.test.ts index 8cb29797a0..3ef235368e 100644 --- a/languageServer/src/test/definitions.test.ts +++ b/languageServer/src/test/definitions.test.ts @@ -7,7 +7,6 @@ import { train_dvc_yaml } from './fixtures/examples/valid' import { params } from './fixtures/params' -import { train } from './fixtures/python' import { requestDefinitions } from './utils/requestDefinitions' import { openTheseFilesAndNotifyServer } from './utils/openTheseFilesAndNotifyServer' import { @@ -58,7 +57,7 @@ describe('textDocument/definitions', () => { expect(response).toBeTruthy() expect(response).toStrictEqual({ - range: Range.create(Position.create(0, 0), Position.create(5, 9)), + range: Range.create(Position.create(0, 0), Position.create(0, 0)), uri: 'file:///params.yaml' }) }) @@ -68,7 +67,7 @@ describe('textDocument/definitions', () => { mockedReadFileContents.mockImplementation(path => { if (path === trainUri) { - return { contents: train } + return { isFile: true } } return null }) @@ -89,7 +88,7 @@ describe('textDocument/definitions', () => { expect(response).toBeTruthy() expect(response).toStrictEqual({ - range: Range.create(Position.create(0, 0), Position.create(7, 13)), + range: Range.create(Position.create(0, 0), Position.create(0, 0)), uri: trainUri }) }) diff --git a/languageServer/src/test/utils/setup-test-connections.ts b/languageServer/src/test/utils/setup-test-connections.ts index 08928f2694..b09e3846c4 100644 --- a/languageServer/src/test/utils/setup-test-connections.ts +++ b/languageServer/src/test/utils/setup-test-connections.ts @@ -24,7 +24,7 @@ export const setupTestConnections = ( server = createConnection(up, down) client = createConnection(down, up) - client.onRequest('readFileContents', mockedReadFileContents) + client.onRequest('isFile', mockedReadFileContents) dvcLanguageService.listen(server) client.listen() diff --git a/languageServer/src/textDocument.ts b/languageServer/src/textDocument.ts index 509582ebea..1c62952351 100644 --- a/languageServer/src/textDocument.ts +++ b/languageServer/src/textDocument.ts @@ -17,19 +17,11 @@ import { visit } from 'yaml' -export const getTextDocumentLocation = ( - textDocument: TextDocument -): Location => { - const start = Position.create(0, 0) - const end = textDocument.positionAt(textDocument.getText().length - 1) - const range = Range.create(start, end) - - return Location.create(textDocument.uri, range) -} +export const getLocationToOpen = (uri: string): Location => { + const position = Position.create(0, 0) + const range = Range.create(position, position) -export const getUriLocation = (uri: string, content: string): Location => { - const textDocument = TextDocument.create(uri, 'plain/text', 0, content) - return getTextDocumentLocation(textDocument) + return Location.create(uri, range) } const yamlScalarNodeToDocumentSymbols = (