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

Dedup diagnostics #1883

Merged
merged 2 commits into from
Apr 5, 2024
Merged

Dedup diagnostics #1883

merged 2 commits into from
Apr 5, 2024

Conversation

jneem
Copy link
Member

@jneem jneem commented Apr 5, 2024

Unfortunately the lsp_types crate doesn't provide a lot of Ord impls, so I had to write some wrappers. Is there a better way? (Of course we could use hashmaps to dedup, but lsp_types also doesn't provide Hash impls)

Fixes #1882

@github-actions github-actions bot temporarily deployed to pull request April 5, 2024 15:56 Inactive
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

I don't know of an obvious better way to do that on a foreign types (which means it's not possible to derive anything automatically either). Although I wonder if we really need the newtype wrappers boilerplate, and if we couldn't just move cmp to free-standing comparison functions instead which we then pass to e.g. sort_by. That might save a few map and unwrapping?

@jneem jneem added this pull request to the merge queue Apr 5, 2024
@jneem jneem removed this pull request from the merge queue due to a manual request Apr 5, 2024
@jneem
Copy link
Member Author

jneem commented Apr 5, 2024

I had a try with this but it had even a little more boilerplate. It would be fine if the lsp_types types were at the "top level", but the Option<Vec<lsp_types::DiagnosticRelatedInformation>> is pretty annoying...

@jneem jneem added this pull request to the merge queue Apr 5, 2024
Merged via the queue into master with commit 1f40fcf Apr 5, 2024
5 checks passed
@jneem jneem deleted the dedup-diagnostics branch April 5, 2024 19:27
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.

NLS reports same diagnostic message multiple times
2 participants