Skip to content

(GH-1413) Update grammar parsing for vscode-textmate v4 module #1414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 10, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 31 additions & 20 deletions src/features/Folding.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
/*---------------------------------------------------------
* Copyright (C) Microsoft Corporation. All rights reserved.
*--------------------------------------------------------*/

import fs = require("fs");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most of the rest of this codebase has a blank line after the copyright. Other than that LGTM. Thanks for fixing this!!

import * as path from "path";
import * as vscode from "vscode";
import {
DocumentSelector,
LanguageClient,
} from "vscode-languageclient";
import { IFeature } from "../feature";
import { Logger } from "../logging";
import { ILogger } from "../logging";
import * as Settings from "../settings";

/**
Expand Down Expand Up @@ -497,23 +497,26 @@ export class FoldingFeature implements IFeature {
* @param logger The logging object to send messages to
* @param documentSelector documentSelector object for this Folding Provider
*/
constructor(private logger: Logger, documentSelector: DocumentSelector) {
const grammar: IGrammar = this.grammar(logger);

constructor(private logger: ILogger, documentSelector: DocumentSelector) {
const settings = Settings.load();
if (!(settings.codeFolding && settings.codeFolding.enable)) { return; }

// If the PowerShell grammar is not available for some reason, don't register a folding provider,
// which reverts VSCode to the default indentation style folding
if (grammar == null) {
logger.writeWarning("Unable to load the PowerShell grammar file");
return;
}
this.loadPSGrammar(logger)
.then((grammar) => {
// If the PowerShell grammar is not available for some reason, don't register a folding provider,
// which reverts VSCode to the default indentation style folding
if (!grammar) {
logger.writeWarning("Unable to load the PowerShell grammar file");
return;
}

this.foldingProvider = new FoldingProvider(grammar);
vscode.languages.registerFoldingRangeProvider(documentSelector, this.foldingProvider);
this.foldingProvider = new FoldingProvider(grammar);
vscode.languages.registerFoldingRangeProvider(documentSelector, this.foldingProvider);

logger.write("Syntax Folding Provider registered");
logger.write("Syntax Folding Provider registered");
}, (err) => {
this.logger.writeError(`Failed to load grammar file - error: ${err}`);
});
}

/* dispose() is required by the IFeature interface, but is not required by this feature */
Expand All @@ -527,7 +530,7 @@ export class FoldingFeature implements IFeature {
* @param logger The logging object to send messages to
* @returns A grammar parser for the PowerShell language is succesful or undefined if an error occured
*/
public grammar(logger: Logger): IGrammar {
public loadPSGrammar(logger: ILogger): Thenable<IGrammar> {
const tm = this.getCoreNodeModule("vscode-textmate", logger);
if (tm == null) { return undefined; }
logger.writeDiagnostic(`Loaded the vscode-textmate module`);
Expand All @@ -537,10 +540,18 @@ export class FoldingFeature implements IFeature {
const grammarPath = this.powerShellGrammarPath();
if (grammarPath == null) { return undefined; }
logger.writeDiagnostic(`PowerShell grammar file specified as ${grammarPath}`);
try {
return registry.loadGrammarFromPathSync(grammarPath);
} catch (err) {
logger.writeError(`Error while loading the PowerShell grammar file at ${grammarPath}`, err);

// Branching for the different vscode-textmate modules
if ("loadGrammarFromPathSync" in registry) {
// V3 of the module allows synchronous loading of a grammar
return new Promise( (grammar) => {
return registry.loadGrammarFromPathSync(grammarPath);
});
} else {
// However in V4+ this is async only
const content = fs.readFileSync(grammarPath);
const rawGrammar = tm.parseRawGrammar(content.toString(), grammarPath);
return registry.addGrammar(rawGrammar);
}
}

Expand All @@ -552,7 +563,7 @@ export class FoldingFeature implements IFeature {
* @param logger The logging object to send messages to
* @returns The required module, or null if the module cannot be required
*/
private getCoreNodeModule(moduleName: string, logger: Logger) {
private getCoreNodeModule(moduleName: string, logger: ILogger) {
// Attempt to load the module from known locations
const loadLocations: string[] = [
`${vscode.env.appRoot}/node_modules.asar/${moduleName}`,
Expand Down
14 changes: 13 additions & 1 deletion src/logging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,19 @@ export enum LogLevel {
Error,
}

export class Logger {
/** Interface for logging operations. New features should use this interface for the "type" of logger.
* This will allow for easy mocking of the logger during unit tests.
*/
export interface ILogger {
write(message: string, ...additionalMessages: string[]);
writeDiagnostic(message: string, ...additionalMessages: string[]);
writeVerbose(message: string, ...additionalMessages: string[]);
writeWarning(message: string, ...additionalMessages: string[]);
writeAndShowWarning(message: string, ...additionalMessages: string[]);
writeError(message: string, ...additionalMessages: string[]);
}

export class Logger implements ILogger {

public logBasePath: string;
public logSessionPath: string;
Expand Down
4 changes: 2 additions & 2 deletions test/features/folding.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ function assertFoldingRegions(result, expected): void {

suite("Features", () => {

suite("Folding Provider", () => {
suite("Folding Provider", async () => {
const logger: MockLogger = new MockLogger();
const mockSelector: DocumentSelector = [
{ language: "powershell", scheme: "file" },
];
const psGrammar = (new folding.FoldingFeature(logger, mockSelector)).grammar(logger);
const psGrammar = await (new folding.FoldingFeature(logger, mockSelector)).loadPSGrammar(logger);
const provider = (new folding.FoldingProvider(psGrammar));

test("Can detect the PowerShell Grammar", () => {
Expand Down
16 changes: 2 additions & 14 deletions test/test_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,9 @@
* Copyright (C) Microsoft Corporation. All rights reserved.
*--------------------------------------------------------*/

import { Logger, LogLevel } from "../src/logging";

export class MockLogger extends Logger {
// Note - This is not a true mock as the constructor is inherited and causes errors due to trying load
// the "PowerShell Extension Logs" multiple times. Ideally logging should be via an interface and then
// we can mock correctly.

public dispose() { return undefined; }

public getLogFilePath(baseName: string): string { return "mock"; }

public writeAtLevel(logLevel: LogLevel, message: string, ...additionalMessages: string[]) { return undefined; }
import { ILogger } from "../src/logging";

export class MockLogger implements ILogger {
public write(message: string, ...additionalMessages: string[]) { return undefined; }

public writeDiagnostic(message: string, ...additionalMessages: string[]) { return undefined; }
Expand All @@ -28,6 +18,4 @@ export class MockLogger extends Logger {
public writeError(message: string, ...additionalMessages: string[]) { return undefined; }

public writeAndShowError(message: string, ...additionalMessages: string[]) { return undefined; }

public startNewLog(minimumLogLevel: string = "Normal") { return undefined; }
}