search

leinardi / mypy-pycharm

A plugin providing both real-time and on-demand scanning of Python files with Mypy from within PyCharm/IDEA.
Apache License 2.0
188 stars 30 forks source link

Improve performance, convert real-time scanning to ExternalAnnotator API #77

Closed intgr closed 2 years ago

intgr commented 2 years ago

Using the ExternalAnnotator API (instead of LocalInspectionTool) behaves a lot better with slow scanners like mypy and fixes reported performance problems with the plugin (fixes #43).

Following multiple successive changes to a file, LocalInspectionTool can invoke the checker for each modification from multiple threads in parallel, which can bog down the system. ExternalAnnotator cancels the previous running check (if any) before running the next one.

Modeled after com.jetbrains.python.validation.Pep8ExternalAnnotator

A few notes:

Contributor checklist

Description

Type of Changes

Type
✓ :bug: Bug fix
✓ :hammer: Refactoring

Related Issue

Closes #43

leinardi commented 2 years ago

Oh wow this seems really interesting! Today I can't check it but I'll try to do it tomorrow, thanks a lot!

leinardi commented 2 years ago

Hi @intgr, I tested your branch and it seems to work fine!

I have some questions:

Inspections from ExternalAnnotator now look different

What does it mean that they look different? Because of the different severity levels?

In the "Problems" window, mypy inspections can no longer be grouped by inspection type -- they always appear at the root level.

I also don't get this: form what I saw the "Problems" window before and after your changes looks the same, beside the different severity level (at least with the project I'm testing): image image

Where can I find this "grooping by inspection type" that you say is not available anymore?

Finally, I saw that this is not checked: image is something else missing?

intgr commented 2 years ago

Some screenshots to illustrate the differences:

In the "Problems" window, mypy inspections can no longer be grouped by inspection type -- they always appear at the root level.

Before After
image image

Inspections from ExternalAnnotator now look different What does it mean that they look different? Because of the different severity levels?

Yeah, the styles associated with the new severity levels are different.

Before After
image image

Finally, I saw that this is not checked: [...] is something else missing?

Not that I know of. I just want to do some additional testing and review myself before I'm confident in this.

intgr commented 2 years ago

Oh one more thing, the intention actions to suppress Mypy errors, disable inspection etc, is no longer available, and PyCharm suppression syntax is ignored.

Before: (this option is no longer available)

image

After: (noinspection comment ignored)

image

I haven't checked how difficult it would be to add back support for this. I guess there are some users relying on this feature. What do you think?

Although it would be better to generate the mypy-native # type: ignore comments instead.

intgr commented 2 years ago

There's also this from comment from JetBrains developers: https://intellij-support.jetbrains.com/hc/en-us/community/posts/115000691050/comments/115000579464

we think of inspections as something which can be disabled, annotations - something which can't, something like compiler errors.

Considering that, the LocalInspectionTool API sounds like a better fit in theory. But fixing the performance issues I think outweighs any argument from purism. Even JetBrains keeps breaking this rule with PEP 8 and TypeScript inspections. I think it's just bad design to have so different APIs for this similar functionality. I have lots of other gripes about the API design as well, but there's nothing we can do about it really. :)

leinardi commented 2 years ago

I haven't checked how difficult it would be to add back support for this. I guess there are some users relying on this feature. What do you think?

Although it would be better to generate the mypy-native # type: ignore comments instead.

Yeah this is not be best but, as you said, would be better to use the mypy ignore comments.

intgr commented 2 years ago

Other than the shortcomings I pointed out in comments and bikeshedding about severity levels, this PR is ready.

leinardi commented 2 years ago

Nice! Would it be OK for you if I add you to the Acknowledge section and to the changelog for your contribution to the project?

intgr commented 2 years ago

Yes of course. Thanks :)

leinardi commented 2 years ago

And do you think a new release can already be made or do you have other changes you want to submit before the next release?

intgr commented 2 years ago

I'm looking into how difficult it would be to create an intention for generating # type: ignore comments. If it's not done by tomorrow, it's probably worth releasing without it.

leinardi commented 2 years ago

Hi @intgr, unfortunately the review process failed due to this error: image This AnnotationBuilder class seems to not be available on any version prior 2020.1.4: image

Shall we just drop those version and change the sinceBuild to 201.8743 or do you have any other suggestion?

intgr commented 2 years ago

Hi! There was an alternative API AnnotationHolder.createAnnotation (instead of newAnnotation()). But that one was marked deprecated.

But I don't think it's common for people to use IDEs that are more than a year old. Personally I wouldn't bother with compatibility.

leinardi commented 2 years ago

Yeah, CheckstyleIDEA is also supporting only recent IDE versions: https://github.com/jshiell/checkstyle-idea/blob/main/src/main/resources/META-INF/plugin.xml#L19

I'll drop everything pre 2020 :+1:

leinardi commented 2 years ago

Version 0.12.1 is out :tada: https://plugins.jetbrains.com/plugin/11086-mypy/versions/stable/148936

intgr commented 2 years ago

Cool, thanks!

Unfortunately because the 0.12.0 version is hidden, the original release notes aren't visible there. :)

leinardi commented 2 years ago

Oh that's a shame, I never had a plugin rejection before so I was not aware of this, sorry :(