crystal-ameba/vscode-crystal-ameba

Ameba 1.5.0 error when config file is not used

joseafga opened this issue · 2 comments

I was getting the message "Ameba: failed to parse JSON response."
Inspecting in the console the error was:

Ameba: failed parsing JSON: SyntaxError: Unexpected token 'E', "Error: Una"... is not valid JSON

Then checking the STDOUT of the command I got the following:

Error: Unable to load config file: Config file does not exist /my/path/to/.ameba.yml

Searching a bit, I see the new Ameba 1.5.0 raises exception when --config argument is used but file (.ameba.yml) does not exist. Apparently this did not happen in previous versions.

Checking how the extension treats the configuration file, I found this condition that I don't quite understand the sense of.

if (this.config.configFileName.length) {
const dir = workspace.getWorkspaceFolder(document.uri)!.uri.fsPath;
args.push('--config', path.join(dir, this.config.configFileName));
}

I think it should be checked if that file exists and not the length of filename.

Possible solution would be:

diff --git a/src/ameba.ts b/src/ameba.ts
index 56b5b43..60474ec 100644
--- a/src/ameba.ts
+++ b/src/ameba.ts
@@ -1,5 +1,6 @@
 import { exec } from 'child_process';
 import * as path from 'path';
+import { existsSync } from 'fs';
 import {
     commands,
     Diagnostic,
@@ -32,10 +33,9 @@ export class Ameba {
         }
 
         const args = [this.config.command, document.fileName, '--format', 'json'];
-        if (this.config.configFileName.length) {
-            const dir = workspace.getWorkspaceFolder(document.uri)!.uri.fsPath;
-            args.push('--config', path.join(dir, this.config.configFileName));
-        }
+        const dir = workspace.getWorkspaceFolder(document.uri)!.uri.fsPath;
+        const configFile = path.join(dir, this.config.configFileName);
+        if (existsSync(configFile)) args.push('--config', configFile);
 
         const task = new Task(document.uri, token => {
             const proc = exec(args.join(' '), (err, stdout, stderr) => {

Ameba::Config.each_config_path already checks if the config file exists in the current directory as well as in the user's home. Perhaps the better approach is to remove this from the extension and let Ameba handle it itself.

@joseafga yes, this is a compatibility bug between VS code plugin and latest ameba release. We may not be able to remove the check completely and rely on Ameba::Config.each_config_path however, the existsSync looks like a valid solution.

Would you prefer opening a PR since you did a lot of investigation work already?

perfect, I'll do the PR

thanks