add simple progress reporting

[syphar]

based on https://github.com/python-lsp/python-lsp-server/pull/236

[syphar]

manually tested together with the current develop branch on python-lsp-server

[rchl]

To avoid changing indentation of hundreds of lines, you could move the original code into a new method (like get_diagnostics or similar) and just call it within the newly added with block.

That said, I'm not sure I agree with using progress indicator in this case. I think that the idea of a progress indicator is more useful for long-running actions that the user has explicitly triggered rather than for something that happens automatically (in this case every time the document is changed). Running it on a document change will IMO be more noisy and annoying than useful as you would see this progress showing up and disappearing constantly.

If you and/or maintainer disagrees with my statement then at least there should be an option to toggle this IMO.

[syphar]

That said, I'm not sure I agree with using progress indicator in this case. I think that the idea of a progress indicator is more useful for long-running actions that the user has explicitly triggered rather than for something that happens automatically (in this case every time the document is changed). Running it on a document change will IMO be more noisy and annoying than useful as you would see this progress showing up and disappearing constantly.

Of course up to maintainers to decide, but for me the mypy lint was the main reason to work on the progress indicators.

In one of the projects I'm working on the mypy check after save takes around 5-10, sometimes 15 seconds (yes, with the daemon), and without the progress indicator I'm often not sure if the error still exists, or the mypy check is just still running.

For things like highlights / autocomplete this is definitely too noisy.

On top of that, in the original PR to the lsp-server (https://github.com/python-lsp/python-lsp-server/pull/236/) I already added the progress reporting to the builtin linter & formatter plugins, so adding mypy too falls into the same line.

[Richardk2n]

A mypy check can be long(ish) running for big files. Therefore, I think this is worthwhile including, but with a config option that is off by default.

[rchl]

Yes, option would be welcome.

Will also ping @ccordoba12 and @krassowski since this discussion is also relevant for the core server.

[syphar]

I'm happy to make this configurable if that what's needed.

Could you point me to a place / PR where I see how this would look like?

[rchl]

Maybe just check how live_mode setting is implemented, for example.

https://github.com/python-lsp/pylsp-mypy/blob/1c5a336cf8eff667de027ecd04ba8d0e230723a3/pylsp_mypy/plugin.py#L169

I wonder if it would make sense to have three options: off, on and only on save. The reasoning for having "on save" would be that save is more explicit action. But maybe that's overkill...

[ccordoba12]

Will also ping @ccordoba12 and @krassowski since this discussion is also relevant for the core server.

From my side I'd like to say to @Richardk2n (this plugin's maintainer) that this requires a release of PyLSP 1.7.0 to work (which I plan to do at the end of next week).

@syphar, could you also add the following constraint to setup.cfg?

...
install_requires =
    python-lsp-server >=1.7.0
...

Otherwise your work looks good to me. But the idea of putting this behind an option (which in my opinion should be True by default) is nice too.

[syphar]

• I added the minimum required version
• I changed the structure to using a separate get_diagnostics method following the proposal of @rchl
• I made the progress reporting configurable, default False, as @Richardk2n said.

[rchl]

But the idea of putting this behind an option (which in my opinion should be True by default)

I'm not really looking forward to my editor flashing progress messages all the time when editing the document. With "live_mode" disabled for pylsp_mypy, it would be fine but otherwise not so much IMO.

Also I believe @syphar said that next version of pylsp will start reporting progress for other linters also. Not a fan of that either. And if that's gonna have a separate progress for every plugin that pylsp runs then that might result in a fun disco.

[ccordoba12]

Also I believe @syphar said that next version of pylsp will start reporting progress for other linters also. Not a fan of that either. And if that's gonna have a separate progress for every plugin that pylsp runs then that might result in a fun disco.

Ok, I see. In that case perhaps we should add a global option to disable all reporting progress in the server itself (I think adding one per linter would be too much work for users to disable them).

@syphar, what do you think?

[Richardk2n]

@ccordoba12 Why do you want them on by default? In general per linter control seems desirable. mypy might run slow in a short file if the project is huge, others will run fast. So I would maybe want no reporting on the fast ones but reporting on the slow ones. Most of them run fast, so off as default and the ability to turn each one on seems very reasonable to me. What is your thinking?

[ccordoba12]

Why do you want them on by default?

I don't have a strong preference about it, but given what you and @rchl said, perhaps we should set it off by default.

In general per linter control seems desirable

Again, no strong preference.

@syphar, what are your thoughts about this?

[syphar]

Personally I'm totally find with useful progress is on by default, like other LSPs I'm using are doing it (like null-ls , rust-analyzer, sumneko-lua).

In general when developing I'm more a friend of "start simple, improve later based on feedback", which, applied to this PR and progress-reporting in general, would mean we have it always on, perhaps with a global switch to enable/disable progress reporting for everything. I would even enable it by default, of course only extrapolated from my preference to the user preference ;) IMO many users would miss this awesome new feature when it's not active by default. Coming from the initial progress reporting PR (https://github.com/python-lsp/python-lsp-server/pull/236 ) and the related autoimport PR (https://github.com/python-lsp/python-lsp-server/pull/305) I would guess that @ccordoba12 would see it similarly.

Adding a config flag for every place where we would start reporting progress feels a little too much to me, and I'm not sure if that's really needed for most of the userbase.

But, now more specific to this PR and the lsp-server: Whatever you as the maintainers feel that fits your goals for the project is fine with me. I'm happy to implement either way, as long as I know which will get this merged :)

[syphar]

@ccordoba12 @Richardk2n @rchl

how can we proceed here?

I'm happy to implement any needed changes on the lsp server and this plugin, as long as I know which way to go that gets this merged in both projects :)

[rchl]

From my side, I would like to test it out and see how it behaves in my editor of choice but I would like python-lsp-server with support for progress notifications to be released first as it would be must easier to test then.

But still my general opinion is that progress notifications are not a good fit for actions that generally take short time to complete or ones that happen all the time on typing. That said, I guess we can't really know whether specific action/plugin will complete quickly since it might depend on a project. In that case I could suggest to maybe only trigger progress notifications for actions that hasn't completed after one second. That would avoid a lot of noise and provide useful feedback for actions that take longer time to run. I know that this might sound a bit disruptive or even premature to add now, before we have even tried the current behavior first...

like other LSPs I'm using are doing it (like null-ls , rust-analyzer, sumneko-lua).

Can you more specifically describe when (for what actions) those servers use progress reporting? I have a little bit of experience with rust-analyzer and as far as I know, it triggers progress reporting on initial loading of the project which is useful indeed since that can take a while but I'm not aware of it reporting progress during normal typing or short-running actions.

[Richardk2n]

I am waiting for the release of the lsp-server, that supports this feature. Merging this now, whould mean I cannot (sensibly) do releases until the server is released, also I agree with the stance of @rchl that it would be a good idea to first use this a little to assure I do not unleash something majorly annoying onto the people.

[ccordoba12]

I just released PyLSP 1.7.0.

[rchl]

Progress reporting not working here. See the https://github.com/python-lsp/python-lsp-server/issues/325 issue.

[rchl]

For the purpose of testing I've hacked Sublime Text LSP to allow those progress notifications to work and as expected, I don't really like how things are flickering in the status field when editing the document:

https://user-images.githubusercontent.com/153197/210016902-50797a13-5ca4-4010-92d3-c0f340f9e258.mov

IMO, actions like renaming a symbol, going to definition/references or formatting a document are OK to have progress reporting but linters that run on editing not so much. I would be fine with those reporting progress if they ran for a while (1s or other arbitrary period).

[Richardk2n]

If nobody objects I would merge and release this as it is now (all I changed is bring it up to date with master and some phrasing in the Documentation). As the video from @rchl shows, having this can often be annoying, therefore I will have it off by default.

Going forward I will think about timing (only report progress if it is already running for n seconds), but for now, in my opinion this is not needed.