martanne / vis

A vi-like editor based on Plan 9's structural regular expressions
Other
4.19k stars 259 forks source link

Open up linter in new window #1070

Closed BioBox closed 11 months ago

BioBox commented 1 year ago

I was testing using these commands for tools like shellcheck and while I had some success it required a bit of inelegance.

In a deliberately broken shell script I ran :|shellcheck "$vis_filepath" only to be told that the Command failed. I was a bit confused by this because of course the command is going to exit non-zero.

In a roundabout of thinking I realised vis may be suppressing output (even stdout) if the command terminates non-zero; the workaround I threw at it was :|shellcheck "$vis_filepath" || true and behold it worked.

I greatly benefit from tools like shellcheck, various static analysers and other lint checkers to make up for my flaws of being human, but vis is making it slightly unobvious as to how this can be done nicely (either via inline annotation like vim's wonderful :makeprg or producing a :new buffer displaying the issues).

Sorry if I'm missed the important documentation but how is such a usecase expected to be handled?

PS: Does vis have a way to express something like vim's <cfile>?

PPS: If the expected approach is to use :| (or :<) and then undo'ing the changes, is it at least possible to make the command output open a :new window? The trouble with opening :new and then running commands is you use vis_filepath AFAIUI.

Originally posted by @Earnestly in https://github.com/martanne/vis/issues/416#issuecomment-300511113

Is there any answer for this?

tosch42 commented 1 year ago

Hi Daniel,

(I've scrambled the patches together rather quickly, so forgive me for potential bugs. They're also only tested very briefly and only intended to serve as examples :)

I was testing using these commands for tools like shellcheck and while I had some success it required a bit of inelegance.

In a deliberately broken shell script I ran :|shellcheck "$vis_filepath" only to be told that the Command failed. I was a bit confused by this because of course the command is going to exit non-zero.

In a roundabout of thinking I realised vis may be suppressing output (even stdout) if the command terminates non-zero; the workaround I threw at it was :|shellcheck "$vis_filepath" || true and behold it worked.

vis relies on a non-zero return value to indicate an error. As you've observed, this is not ideal. The problem here is that different developers understand different things under the words "failed" and "succeeded". For shellcheck it is reasonable to return different values for different "errors". The problem is that one of those values doesn't represent a "real" error within the program itself. That is, 1. I understand why they chose to return a non-zero value here, but that's exactly the part where we run into problems. vis would need huge test cases for different programs that return different values. I think I don't need to explain to you that this is fragile and quite frankly sucks :-)

In case your not familiar with the source code, take a quick look at sam.c:1732 With the following patches I try to clarify the inner workings a bit. They're not meant to fix your problem (and if they somehow do, that's great :), just to demonstrate some issues.

diff --git a/sam.c b/sam.c
index 29e9c583df2f..5a02ad05a17e 100644
--- a/sam.c
+++ b/sam.c
@@ -1741,13 +1741,11 @@ static bool cmd_filter(Vis *vis, Win *win, Command *cmd, const char *argv[], Sel

    if (vis->interrupted) {
        vis_info_show(vis, "Command cancelled");
-   } else if (status == 0) {
+   } else {
        size_t len = buffer_length(&bufout);
        char *data = buffer_move(&bufout);
        if (!sam_change(win, sel, range, data, len, 1))
            free(data);
-   } else {
-       vis_info_show(vis, "Command failed %s", buffer_content0(&buferr));
    }

    buffer_release(&bufout);

With this patch applied, we remove the distinction between programs that failed and those that did not (in terms of return values). Now vis stops suppressing stdout, even on program "failure". The issue that arises here is obvious. If you enter a command such as :|cat noway, where noway does not exist, vis just silently does nothing instead of providing an error message. As the default behaviour, that's irritating IMO.

My second idea was something like this:

diff --git a/sam.c b/sam.c
index 29e9c583df2f..ef098e5eae91 100644
--- a/sam.c
+++ b/sam.c
@@ -1741,14 +1741,13 @@ static bool cmd_filter(Vis *vis, Win *win, Command *cmd, const char *argv[], Sel

    if (vis->interrupted) {
        vis_info_show(vis, "Command cancelled");
-   } else if (status == 0) {
-       size_t len = buffer_length(&bufout);
-       char *data = buffer_move(&bufout);
-       if (!sam_change(win, sel, range, data, len, 1))
-           free(data);
-   } else {
+   } else if (status != 0) {
        vis_info_show(vis, "Command failed %s", buffer_content0(&buferr));
    }
+   size_t len = buffer_length(&bufout);
+   char *data = buffer_move(&bufout);
+   if (!sam_change(win, sel, range, data, len, 1))
+       free(data);

    buffer_release(&bufout);
    buffer_release(&buferr);

This preserves the current behaviour but stdout is always inserted. Thus, vis still states that "Command failed" in case of a return value unequal to 0 but stdout is inserted. This might increase the confusion for the user since it seems like the command succeeded but vis tells him/her the opposite. I can already see the bug-reports :-) Another thought that came to my mind was to change the error message from "Command failed" to something else. Perhaps something more general like "Command returned %d: %s". I couldn't come up with a good new message, suggestions are welcome.

TL;DR As long as we compare return values, false positives are inevitable. And I don't think there is a good way to check if a program failed without looking at the return value :-)

So if you really want to avoid the "Command failed" message, either use the || true trick, write a wrapper script, do some :! magic or use a patch similar to the ones above.

I greatly benefit from tools like shellcheck, various static analysers and other lint checkers to make up for my flaws of being human, but vis is making it slightly unobvious as to how this can be done nicely (either via inline annotation like vim's wonderful :makeprg or producing a :new buffer displaying the issues).

I just use tmux for this, but I know that "use something else" is a bad answer. Just wanted to throw it in, in case you can't find another solution.

Sorry if I'm missed the important documentation but how is such a usecase expected to be handled?

PS: Does vis have a way to express something like vim's ?

Although I've used vim for a good amount of time, I don't recognize this feature. Could you describe its behaviour a bit further?

PPS: If the expected approach is to use :| (or :<) and then undo'ing the changes, is it at least possible to make the command output open a :new window? The trouble with opening :new and then running commands is you use vis_filepath AFAIUI.

I'm not aware that stock-vis is able to do that. However, I think this should definitely be possible. It would be great if one could pipe text between windows and thus create some sort of transparent windowing-system. The ability to pass an argument to :new would be great, too. A Lua interface for this would be very useful. I can imagine a lot of plugins making use of it. However, this should be discussed in a different thread. I also haven't looked at the code to see what restrictions there are.

EDIT: syntax highlighting

BioBox commented 1 year ago

Thank you very much for the lovely response - it was very informative. That entire first post was copied from a previous discussion and was very close to what I want, which is to use a linter with vis in a way that isn't painful. Much to my dismay the answer to the PPS section is a "No".

This doesn't have much to do with the subject, but to answer your question <cfile> is a special token that one can enter into the command line which will be expanded into the current path under the cursor. There are many other such examples, like how % is vim's version of $vis_filepath. Easier to type too.

But yeah, your last paragraph is right on the money. I'd have to stick with vim even if vis did have that feature because it's just too good. I've been in an abusive love/hate relationship with it for many years now, but it's indisputably the greatest text editor to be conceived by man at the time of this writing. However I do have hope that one day a modern vis fork will rise to challenge vim, if only to vanquish vimscript if nothing else.

rnpnr commented 1 year ago

I think the current behavior and usage of :< and :| is clearly defined and makes perfect sense as it is. vis isn't supposed to emulate every aspect of vim.

That being said if I understand what you are trying to do correctly its very easy to implement using the lua interface. Here is something I quickly wrote up that will allow you to run the command :lint and open the output in the message window:

vis.events.subscribe(vis.events.INIT, function()
    vis:command_register("lint", function()
        local linters = {}
        linters["bash"] = "shellcheck"

        local cmd = linters[vis.win.syntax]
        if cmd == nil then
            vis:info(vis.win.syntax .. ": linter not defined")
            return false
        end

        local file = vis.win.file
        local _, ostr, estr = vis:pipe(file, {start = 0, finish = 0}, cmd .. " " .. file.name)
        if estr then
            vis:message(estr)
            return false
        end
        vis:message(ostr)
        return true
    end)
end)

This will try to autodetect the file type using the built in method for deciding which syntax highlighter to use and then run the defined linter on the current file. You can add more filetypes and linters to the linters table as needed. The filetype names come from the lexers directory.

roguh commented 1 year ago

Do you mind if I put that in a plugin?

rnpnr commented 1 year ago

Do you mind if I put that in a plugin?

I didn't think anyone actually wanted to use this. I made a repo here. Feel free to submit a pull request with additional linters if you want.

roguh commented 1 year ago

It's useful to me. I added some more features, will get you a PR soon.

rnpnr commented 11 months ago

I'm going to close this since its now available with the plugin linked here and above.