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

feat: add option to disable/enable diagnostics in hover #2489

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

HJX-001
Copy link
Contributor

@HJX-001 HJX-001 commented May 31, 2024

fixes issue #2381

plugin/hover.py Outdated Show resolved Hide resolved
@predragnikolic
Copy link
Member

Hello,

The new setting should also be added to https://github.com/sublimelsp/LSP/blob/main/sublime-package.json

@predragnikolic
Copy link
Member

LSP has goto next/previous diagnostics command:

// "command": "lsp_prev_diagnostic",

// "command": "lsp_next_diagnostic",

Currently, when we navigate to a diagnostics, the hover popup is shown with only diagnostics.

sublime.set_timeout_async(lambda: view.run_command('lsp_hover', {'only_diagnostics': True, 'point': diag_pos}), 200)

I'm on a phone now,
So I can't test this at the moment, but I assume that the diagnostics will not be show in the hover popup in that case with this PR.

I would suggest still showing the diagnostics in the hover popup
when navigating diagnostics with the two commands above.

@HJX-001
Copy link
Contributor Author

HJX-001 commented Jun 1, 2024

I have near zero python experience, and I dont have much idea of this codebase. Turn down this pr if not suitable and the issue can be closed now as It is working for me at least.

@HJX-001
Copy link
Contributor Author

HJX-001 commented Jun 1, 2024

Its my first pr so i m not very aware of workflow. It is requested to take over or guide me about 'suggested change'/ any other thing.

@predragnikolic
Copy link
Member

The PR is good, just needs some additional things.

It is ok if you don't know the codebase,
And if you missed a few spots,
that's why code review exist 🙂

Fell free just to push additional commits.(There is no need to close and open a new PR)

If you have additional questions, feel free to ask.

@HJX-001
Copy link
Contributor Author

HJX-001 commented Jun 1, 2024

LSP has goto next/previous diagnostics command:

// "command": "lsp_prev_diagnostic",

// "command": "lsp_next_diagnostic",

Currently, when we navigate to a diagnostics, the hover popup is shown with only diagnostics.

sublime.set_timeout_async(lambda: view.run_command('lsp_hover', {'only_diagnostics': True, 'point': diag_pos}), 200)

I'm on a phone now,
So I can't test this at the moment, but I assume that the diagnostics will not be show in the hover popup in that case with this PR.

I would suggest still showing the diagnostics in the hover popup
when navigating diagnostics with the two commands above.

I tested 'next/prev diagnostics', seems like you were right about it. It is not working. It feels intutive have this working. But on the other hand someone turning off 'diagnostics' in hover should know about it. I'll try to figure out a way for turning 'diagnostics' off for hover only, not for 'next/prev diagnostics'.

@HJX-001
Copy link
Contributor Author

HJX-001 commented Jun 2, 2024

added a new commit that seems resolving 'lsp prev/next diagnostics cmd' not working and added suggested changes and added entry in 'sublime-package.json'. It is requested to review updated changes

Copy link
Member

@predragnikolic predragnikolic left a comment

Choose a reason for hiding this comment

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

Looks good

plugin/hover.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@HJX-001 HJX-001 left a comment

Choose a reason for hiding this comment

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

looks good

@predragnikolic predragnikolic merged commit 7554363 into sublimelsp:main Jun 4, 2024
8 checks passed
@predragnikolic
Copy link
Member

Thank you for the PR

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.

3 participants