Making linter failure easier to find

[oalders]

This is quite possibly my fault, but I wanted to get your input on it. Today I finally sat down to debug why perlimports was not working on my $work machine. In my PerlNavigator setup I was setting

perlimportsProfile = 'perlimports.toml'

However, at $work the file is called .perlimports.toml. At the command line, using a config file which does not exist has the script exit non-zero.

$ perlimports --lint --config-file perlimports.toml foo.pl
perlimports.toml not found at /opt/perl5.38.2/lib/site_perl/5.38.2/App/perlimports/CLI.pm line 293.
$ echo $?
1

That gets ignored silently (as far as I can tell) and PerlNavigator didn't have any messages in the LSP log about this. (I'm using neovim).

My guess is that maybe it's hard in this case to distinguish the perlimports failure to start and an actual linting error. Do you have any suggestions for me on how to deal with this? Should perlimports emit something more helpful in this case? I do now have the --json option, but this exits even before JSON can be emitted. I could emit appropriate JSON if we enabled this flag in Perl Navigator. I'd be interested to hear your thoughts.

[bscan]

Hi @oalders, I am able to reproduce this issue by setting a perlimports profile to a non-existent file:

Do you have logging enabled? perlnavigator.logging? By default, this is disabled for neovim and other non-vscode editors.

If so, you should see Attempted to run perlimports lint:, although no meaningful errors is actually shown. This is at least coming from the error handling section of the code, but only logging stdout not stderr.

nLog("Attempted to run perlimports lint: " + error.stdout, settings);

As another example, when perlimports is not installed or not the correct version, it does print the following helpful error:

Attempted to run perlimports lint: 
Unable to run perlimports as it is not installed

Part of the issue is in the perlImportsWrapper, that prints to STDERR when perlimports fails: (https://github.com/bscan/PerlNavigator/blob/1f3e38e74277f1f95b6a4a10012a71eb9826b109/server/src/perl/perlimportsWrapper.pl#L36 but prints to stdout for other issues:https://github.com/bscan/PerlNavigator/blob/1f3e38e74277f1f95b6a4a10012a71eb9826b109/server/src/perl/perlimportsWrapper.pl#L15

I think the easiest way to handle it is simply having the Perl catch block print to stdout instead since perlimports appears to print directly to stderr when run. I'm happy to make this change if you'd like, or open to other ideas.

[oalders]

I think the easiest way to handle it is simply having the Perl catch block print to stdout instead since perlimports appears to print directly to stderr when run. I'm happy to make this change if you'd like, or open to other ideas.

Thanks for this helpful information. I didn't know about perlnavigator.logging. I will make note of that for future. Your suggestion works for me!

[WhoIsSethDaniel]

I do know about the logging option, but I was thrown off by its documentation:

"perlnavigator.logging": {
      "scope": "resource",
      "default": true,
      "description": "Log to stdout from the navigator. Viewable in the Perl Navigator LSP log",
      "type": "boolean"
    },

Perhaps the description can be changed to mention that it is turned off by default for non-vscode editors? Or maybe this is documented somewhere else.

[bscan]

Thanks @WhoIsSethDaniel. How would you feel about logging being enabled by default? The original reason I disabled it was somewhat silly. I was originally logging to stdout using console.log instead of writing to stderr. This worked fine in vscode because it uses node-ipc as the communication method to the language server. However, all the other editors use stdio by default, so any use of console.log will interleave with the communication protocol and break things, depending on how error tolerant the client is. Switching logging to stderr solved this issue, but I also turned off logging entirely by default, which was overkill.

[WhoIsSethDaniel]

@bscan that doesn't bother me. I'd prefer that. It may be surprising to others when/if they are inspecting their lsp.log and discover these new messages. But maybe it's the good kind of surprise?

[WhoIsSethDaniel]

121 is, afaik, the fix for the only take-away from this thread. I'll make a different bug report for the logging.

[oalders]

Thanks, @WhoIsSethDaniel!