Skip to content
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

fix/breaking: issues with spawned procs, stdin use, env, output #56

Closed
Closed
Show file tree
Hide file tree
Changes from all 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
1 change: 0 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ DBT setup requires these settings to lint and format the document.

```json
"sqlfluff.linter.run": "onSave",
"sqlfluff.experimental.format.executeInTerminal": true,
```

### Format file
Expand Down
7 changes: 1 addition & 6 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,6 @@
"default": true,
"description": "Determines if the document formatter is enabled."
},
"sqlfluff.experimental.format.executeInTerminal": {
"type": "boolean",
"default": false,
"markdownDescription": "Determines if the `sqlfluff fix` command overwrites the file contents instead of this extension. You should not change the file contents while formatting is occurring if this is enabled. May lead to problems if `editor.formatOnSave = true`. This allows formatting to work when the templater is set to dbt. This can help solve [Mojibake](https://en.wikipedia.org/wiki/Mojibake) issues."
},
"sqlfluff.linter.run": {
"type": "string",
"enum": [
Expand All @@ -117,7 +112,7 @@
"default": "onType",
"description": "Determines if the linter runs on save, on type, or disabled.",
"markdownEnumDescriptions": [
"Run the linter when the document is typed in. Does not work when the templater is set to dbt.",
"Run the linter when the document is typed in. Does not work when the templater is set to dbt, does not evaluate .sqlfluff files nested deeper than the root.",
"Run the linter when the document is saved.",
"Do not run the linter."
]
Expand Down
9 changes: 5 additions & 4 deletions src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ import * as vscode from "vscode";
import { EXCLUDE_RULE, SQLFLuffDocumentFormattingEditProvider, SQLFluffLinterProvider, SQLFluffQuickFix } from "./features/sqlFluffLinter";

export const activate = (context: vscode.ExtensionContext) => {
new SQLFluffLinterProvider().activate(context.subscriptions);
const outputChannel = vscode.window.createOutputChannel("SQLFluff");
new SQLFluffLinterProvider(outputChannel).activate(context.subscriptions);

vscode.languages.registerDocumentFormattingEditProvider("sql", new SQLFLuffDocumentFormattingEditProvider().activate());
vscode.languages.registerDocumentFormattingEditProvider("sql-bigquery", new SQLFLuffDocumentFormattingEditProvider().activate());
vscode.languages.registerDocumentFormattingEditProvider("jinja-sql", new SQLFLuffDocumentFormattingEditProvider().activate());
vscode.languages.registerDocumentFormattingEditProvider("sql", new SQLFLuffDocumentFormattingEditProvider(outputChannel).activate());
vscode.languages.registerDocumentFormattingEditProvider("sql-bigquery", new SQLFLuffDocumentFormattingEditProvider(outputChannel).activate());
vscode.languages.registerDocumentFormattingEditProvider("jinja-sql", new SQLFLuffDocumentFormattingEditProvider(outputChannel).activate());

context.subscriptions.push(
vscode.languages.registerCodeActionsProvider("sql", new SQLFluffQuickFix(), {
Expand Down
14 changes: 4 additions & 10 deletions src/features/Helpers/configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,32 +69,26 @@ export class Configuration {
.get("enabled");
}

public static executeInTerminal(): boolean {
return vscode.workspace
.getConfiguration("sqlfluff.experimental.format")
.get("executeInTerminal");
}

public static runTrigger(): string {
return vscode.workspace
.getConfiguration("sqlfluff.linter")
.get("run");
}

public static lintBufferArguments(): string[] {
return ["lint", "--format", "json", "-"];
return ["--format", "json"];
}
Comment on lines 78 to 80
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed and just use lintFileArguments instead as they now return the same values.

Copy link
Author

Choose a reason for hiding this comment

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

so, i was conflicted about this. the issue in my mind is that some people may have modified these already, and removing one might cause unintended side effects. i'm definitely more interested in cutting them down, so if thats the way you'd like to go happy to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite follow? While these are in the configuration.ts file, they can't actually be modified by the user. These arguments should stay consistent. And it looks like you moved the "-" part of it to the section where you are evaluating if the linting should be completed with stdin or not. So we should be okay removing this and any places that use it could just use the lintFileArguments


public static lintFileArguments(): string[] {
return ["lint", "--format", "json"];
return ["--format", "json"];
}

public static formatBufferArguments(): string[] {
return ["fix", "--force", "-"];
return ["--force"];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed as it is no longer used.


public static formatFileArguments(): string[] {
return ["fix", "--force"];
return ["--force"];
}

public static extraArguments(): string[] {
Expand Down
100 changes: 0 additions & 100 deletions src/features/formatter/formattingProvider.ts

This file was deleted.

60 changes: 0 additions & 60 deletions src/features/formatter/process.ts

This file was deleted.

75 changes: 75 additions & 0 deletions src/features/providers/format.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
"use strict";

import * as cp from "child_process";
import * as fs from "fs";
import * as path from "path";
import * as vscode from "vscode";

import { Configuration } from "../helpers/configuration";
import { SQLFluff, SQLFluffCommand } from "../utils/sqlfluff";

export class DocumentFormattingEditProvider {
public outputChannel: vscode.OutputChannel;
public runner: SQLFluff;

constructor(outputChannel: vscode.OutputChannel) {
this.outputChannel = outputChannel;
this.runner = new SQLFluff(this.outputChannel);
}
async provideDocumentFormattingEdits(
document: vscode.TextDocument
): Promise<vscode.TextEdit[]> {
const filePath = document.fileName.replace(/\\/g, "/");
const rootPath = vscode.workspace.workspaceFolders[0].uri.fsPath.replace(/\\/g, "/");
const workingDirectory = Configuration.workingDirectory() ? Configuration.workingDirectory() : rootPath;
const textEdits: vscode.TextEdit[] = [];
this.outputChannel.appendLine(`Format triggered for ${filePath}`);

if (Configuration.formatEnabled()) {
if (document.isDirty) {
await document.save();
}
Comment on lines +28 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason the sqlfluff.experimental.format.executeInTerminal was used here and still marked as experimental was that using directly executing the file would result in the file getting overwritten on disk. This can lead to an error if the user changes the file while the formatting has not completed yet. Do you have any ideas for how to fix that bug?


try {
const result = await this.runner.run(
workingDirectory,
SQLFluffCommand.FIX,
Configuration.formatFileArguments(),
{
targetFileFullPath: filePath,
},
);
if (!result.succeeded) {
throw new Error('Command failed to execute, check logs for details');
}
const contents = fs.readFileSync(filePath, "utf-8");
const lines = contents.split(/\r?\n/);
const lineCount = document.lineCount;
const lastLineRange = document.lineAt(lineCount - 1).range;
const endChar = lastLineRange.end.character;

if (lines[0].startsWith("NO SAFETY:")) {
lines.shift();
lines.shift();
}

if (lines[0].includes("ENOENT")) {
return [];
}

if (lines.length > 1 || lines[0] !== "") {
textEdits.push(vscode.TextEdit.replace(
new vscode.Range(0, 0, lineCount, endChar),
lines.join("\n")
));
}
} catch (error) {
vscode.window.showErrorMessage(`SQLFluff Formatting Failed: ${error.message}`);
}
} else {
this.outputChannel.appendLine("Skipping format, not enabled in settings");
}

return textEdits;
}
}
Loading