Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear diagnostic results on documents #123

Merged
merged 4 commits into from
Aug 23, 2023

Conversation

samskiter
Copy link
Contributor

@samskiter samskiter commented Aug 17, 2023

When a document is scanned and no errors are returned, no diagnostic is reported to the LSP_SERVER. This change checks for diagnostics against the specific file. If none are created, it clears the errors on that file by reporting an empty set of diagnostics.

Tested with unit tests, but not installed and tested yet...

Fixes: #119

When a document is scanned and no errors are returned, no diagnostic is reported to the LSP_SERVER. This change checks for diagnostics against the specific file. If none are created, it clears the errors on that file by reporting an empty set of diagnostics.

Fix for: microsoft#119
@samskiter
Copy link
Contributor Author

@karthiknadig would appreciate a look over if you can? more details on the bug

@karthiknadig
Copy link
Member

karthiknadig commented Aug 17, 2023

@samskiter Thanks for looking into this issue. I have a few questions.
I think a fix would be to track problems in dictionary in both file and workspace scope cases. Use that to clear on a per document basis. what do you think?

I am actually thinking of actually switching this to use a different request: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_pullDiagnostics
As that handles both workspace and file scoped diagnostics and uses a tracking id to refer when it needs to re-check for diagnostics. We will still need a dictionary and tract the problems reported, but this will give us better context on what was reported to the IDE and what the IDE presenting to the user.

For debugging you can actually use the test view to debug specific tests:
image

@samskiter
Copy link
Contributor Author

@karthiknadig Agree that the architecture doesn't quite look right for workspace reporting setting - as you point out, this fix would be unable to clear documents that aren't requested directly from the LSP (i.e. those from included documents, the mypy response doesn't include a list of files it scanned it seems)

I believe that the proposed fix would at least remove the most annoying/frustrating aspects of this bug - that you can edit a file and have it go from Red to Green without having to restart the mypy-type-checker server (which is currently in my muscle memory!). It should be fine for the file-level reporting and the workspace reporting scope is still market experimental by you right? So this might be a good stop-gap while you undertake a bigger refactor as you mention (should probably be tracked in a separate ticket, IMO)? WDYT?

I'm going to also have a quick look to see if there's a way to get mypy to give a response for every file it inspected... might make this even easier to fix up?

@karthiknadig
Copy link
Member

I agree with this as a stop gap fix.

Created #124 to track the bigger change.

karthiknadig
karthiknadig previously approved these changes Aug 17, 2023
roblourens
roblourens previously approved these changes Aug 17, 2023
@samskiter
Copy link
Contributor Author

Also raised: python/mypy#15902 which might make implementing this a little easier

@samskiter samskiter dismissed stale reviews from roblourens and karthiknadig via eeede0f August 18, 2023 09:20
@samskiter
Copy link
Contributor Author

@microsoft-github-policy-service agree

@samskiter
Copy link
Contributor Author

Need a Bug label adding please @karthiknadig

@karthiknadig karthiknadig self-assigned this Aug 18, 2023
@karthiknadig karthiknadig added the bug Issue identified by VS Code Team member as probable bug label Aug 18, 2023
@karthiknadig karthiknadig requested review from karrtikr and anthonykim1 and removed request for roblourens August 18, 2023 16:47
Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint is failing.

@samskiter
Copy link
Contributor Author

@karrtikr fixed :)

@anthonykim1
Copy link

Seems to be still failing.. Did you forget to commit all changes by chance?

@samskiter
Copy link
Contributor Author

i formatted the file with black - which is what I thought was failing

@samskiter
Copy link
Contributor Author

ah, there was a second file that needed formatting. pushed

@samskiter
Copy link
Contributor Author

OK think it's good to go now @anthonykim1

@samskiter
Copy link
Contributor Author

plz can haz review?

@karrtikr karrtikr removed their request for review August 21, 2023 22:02
@samskiter
Copy link
Contributor Author

Just need 1 more review and then we can merge?

@karthiknadig karthiknadig enabled auto-merge (squash) August 23, 2023 17:27
@karthiknadig karthiknadig merged commit 4529f48 into microsoft:main Aug 23, 2023
18 checks passed
@samskiter
Copy link
Contributor Author

Thanks all! 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error message persists after fix/save
5 participants