fix/breaking: issues with spawned procs, stdin use, env, output #56
Conversation
| public static lintBufferArguments(): string[] { | ||
| return ["lint", "--format", "json", "-"]; | ||
| return ["--format", "json"]; | ||
| } |
There was a problem hiding this comment.
This can be removed and just use lintFileArguments instead as they now return the same values.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 formatBufferArguments(): string[] { | ||
| return ["fix", "--force", "-"]; | ||
| return ["--force"]; | ||
| } |
There was a problem hiding this comment.
This can be removed as it is no longer used.
| if (Configuration.formatEnabled()) { | ||
| if (document.isDirty) { | ||
| await document.save(); | ||
| } |
There was a problem hiding this comment.
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?
| this.process.kill('SIGKILL'); | ||
| this.process = undefined; | ||
| } | ||
| const workspaceFolderRootPath = path.normalize(vscode.workspace.workspaceFolders[0].uri.fsPath); |
There was a problem hiding this comment.
Looks like we normalize the paths differently in the lint.ts and format.ts files. Could we change it, either here or in those files, so that we have consistent path normalization?
|
These changes look good. I left a few comments and my main concern would be issues with executing all the formatting directly on the file instead of passing stdin when possible. The formatting can take some time and it is possible that the user will change the file while formatting is happening. When passing in stdin, the formatting is basically stopped and no changes are passed to the file. However, it is possible to automatically overwrite the file with changes instead of always getting an error message, so adding that note to the README might be sufficient. I like the addition of logging everything to an output channel. That should help with identifying problems with the extension. Don't worry about the testing not working, there are a few changes I need to make to the package.json scripts to ensure everything is added to the out directory correctly. And the actual tests probably need to be updated to ensure they work correctly. One other note, could you run the eslint command |
|
I appreciate the quick feedback! I'll take a look at this tonight and have a revision for you tomorrow most likely. I have found a way to fix the experimental command so that it doesnt throw conflicts anymore, I didnt realize that was the issue but now that you mention it i've found a fix. I'll also run the linting command, sorry my editor seems to have not picked up on the existence of a config to identify any issues while coding. It's definitely not perfect when it comes to the timing of certain things like format start -> change made -> format finished as you mentioned, but I dont think there's a way around that without modifications to sqlfluff core unfortunately. The ideal world is that sqlfluff core can accept multiple config files, or when it read from stdin it can also take a file path to do traversal on, but without that we're unfortunately pretty limited to "best of the bad" type of situations. IMO it's better to have a couple rough edges like the "race" condition mentioned than to ignore the nested sqlfluff config file features though, as that's likely to affect people in a more critical way than the race condition they can wait a second or two for. |
|
@timcosta Are you still planning on updating this PR so we can try to get these changes merged? |
|
Yes, work has just had me slammed. |
Ok, no rush. |
Several changes here, some of which are likely high urgency bugfixes, some of which are quality of life, and some are just cleanup. You may view this as a rewrite/major version, I probably would. Solving some of the issues necessitated larger structural changes.
Linting
onTypewas entirely broken if you used a.sqlfluffconfig file anywhere but the workspace root, i.e deeper in the file tree. This was due to the usage of stdin for passing the file contents, which "hid" the file path tree from sqlfluff. I'll give an example because this one is a bit hard to understand:/my/workspace/src/sql/blah.sqlcd /my/workspace && sqlfluff lint --options - < "sql_string".sqlflufffile in either/my/workspace/srcor/my/workspace/src/sqlthat would be picked up by running the command in your terminal/in fix mode wouldn't be detected and merged in the root.sqlflufffile.The list of changes is as follows:
executeInTerminaloption, all formats/fixes are executed in the terminal now due to the nested .sqlfluff issueoptions.envto the child process was preventing sqlfluff from being located in the PATH, requiring you to hardcode the location always. It didn't seem like the old env setting was doing anything, so I removed it which allows the env to pass throughThis has been fully tested with both
onTypeandonSavesettings locally, however the tests in this project are not passing. They also aren't passing in master though so I wasnt too concerned about fixing that. It seems like a fairly involved change due to how the breakage is happening, I tried some basic fixes but I think it's going to require a bit of a test rewrite to handle the promise errors and some other bits like .sql files not being copied into the out dir the tests are run from.If you don't want to merge this PR, that's absolutely fine, I understand that it's effectively a rewrite/breaking change. I will likely publish this PR on my own in that case though so that there's a version of the extension with the above features.