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

Fix diagnostic notifications override on clients #185

Closed
wants to merge 2 commits into from

Conversation

bmeneg
Copy link

@bmeneg bmeneg commented Jul 9, 2023

This PR fixes the diagnostic notifications getting overridden on clients due to multiple notifications for the same file with different diagnostic results and also notifications being sent twice for each file due to a erroneous nested loop.

Fixes #184

On diagnostics notification there is the `-` file that was not being
translated to the real filename when selecting the diagnostics results from
a %hash<filename => diag>, causing a notification with erroneous diagnostics
(empty, as no errors existed for filename `-`), overriding any other
notification for that same file depending on the order of delivery to the
client.

This patch adds just a simple reassignment to $filename using the $uri as
basis, since it was correct even for the `-` filename.
On sending diagnostic notifications two exact same for-loops were nested on
each other, causing duplicated notifications on clients. This commit fixes
it by removing one of the loops.
@bmeneg
Copy link
Author

bmeneg commented Jul 18, 2023

@richterger this PR solves a simple bug with an obvious fix, it would help many using both vscode and vim/nvim through coc-perl. Could you take a look?

@richterger
Copy link
Owner

Using always the uri is not an good idea, also it works most the time.
I have done a different implementation in 5ddf9c6. Please check if this works for you

@bmeneg
Copy link
Author

bmeneg commented Jul 21, 2023

@richterger I dropped a comment in #184, please check it there.

@bmeneg
Copy link
Author

bmeneg commented Jul 23, 2023

This PR was superseded by commits 5ddf9c6 and 27c71b9 integrated directly to master.

@bmeneg bmeneg closed this Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Different notifications for the same syntax check error
2 participants