amperser / proselint

A linter for prose.
http://proselint.com
BSD 3-Clause "New" or "Revised" License
4.36k stars 179 forks source link

when there is an error, `proselint --json` should output the error to `stderr` rather than `stdout` #1307

Open milanglacier opened 2 years ago

milanglacier commented 2 years ago

I am using proselint with null-ls which is a neovim plugin, essentially what this plugin does is cat xxx.md | proselint --json (assuming I am editing a markdown file), sometimes what proseling output will be not a valid json, i.e something like this:

output: Error ([Errno 35] Resource temporarily unavailable: '/Users/northyear/.cache/proselint/checks.airlinese.misc.check') opening /Users/northyear/.cache/proselint/checks.airlinese.misc.check - will attempt to delete and re-open.

which may from here, https://github.com/amperser/proselint/blob/ea8c5749d4ea4fe1a137e45885254e71cbcefa98/proselint/tools.py#L65

i.e from this line. Since it is not a legal json, and then null-ls will parse it with error. It would be better that when --json is supplied, errors should be outputed to stderr rather than stdout.

jose-elias-alvarez commented 2 years ago

To provide a little additional detail here: null-ls is a plugin that spawns external processes like proselint, captures their output, and converts that output into Neovim's diagnostic format for display in the editor. In this case, the issue stems from the error message, which is sent to stdout and therefore breaks the plugin's attempts to parse it as JSON.

Ideally, running proselint with the --json flag would always output valid JSON. I'm not a proselint user myself, so I haven't done research into other integrations, but I imagine they'd run into the same problem if they run under the conditions that create the error message. A simple solution (and one that I've seen other linters use) is to output error messages like these to stderr, which will fix things on our end while still providing feedback to users of the CLI. I'm not a Python expert, but I'm happy to put in a PR if there's a chance of getting this fixed.

Edit: also, if it's of any help, here is the relevant issue on our end, which contains more details (though we haven't been able to figure out what exactly triggers the error in the first place).

Nytelife26 commented 2 years ago

To provide a little additional detail here: null-ls is a plugin that spawns external processes like proselint, captures their output, and converts that output into Neovim's diagnostic format for display in the editor. In this case, the issue stems from the error message, which is sent to stdout and therefore breaks the plugin's attempts to parse it as JSON.

I use null-ls with Neovim on a daily basis, but thank you. It's an honour to speak with you.

This is strange behaviour on part of proselint indeed. I picked up the project a while ago, so I haven't yet squashed all the bugs. As you and @milanglacier have suggested, I'll look into why proselint sometimes fails to produce valid JSON, and also consider throwing to stderr.

milanglacier commented 2 years ago

I find that if proselint is installed by the python installed by homebrew, then it will continuously give me such warning, i.e it can not open the file in ~/.cache/proselint folder. However, if I install proselint via a venv in my home and not use the python installed by homebrew (in fact the python from miniforge) and then it works fine and no warning.

milanglacier commented 2 years ago

It seems that [Errno 35] Resource temporarily unavailable is usually a macOS only problem in many scenarios😂

Nytelife26 commented 2 years ago

It seems that [Errno 35] Resource temporarily unavailable is usually a macOS only problem in many scenarios😂

Odd, I suspected it would be a filesystem issue persistent with most modern UNIX systems. Alas, I should expect nothing less from MacOS 😂 I'll investigate later tonight. Fortunately we have MacOS available in GitHub actions so I can test it that way.

mcepl commented 2 years ago

Actually, it doesn’t seem to be reproducible on Linux as well.

Using neovim nightly (commit 62916946), proselint 0.13.0, null-ls actual).

mabuchner commented 1 year ago

I find that if proselint is installed by the python installed by homebrew, then it will continuously give me such warning, i.e it can not open the file in ~/.cache/proselint folder.

I think, it's a permission issue. On my MacOS system the permissions for ~/.cache/proselint were set to drwx--x--x.

After changing the permissions

chmod g+r,o+r proselint

the error went away.

Of course, it would still be nice, if proselint would output the error message to stderr.

Edit: Spoke too soon. The problem still occurs, but in my case less frequently.

simon-liebehenschel commented 1 year ago

The error happens when proselint is used via pre-commit. Tested on Debian image with a root only user.