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

Improve no plugin messages #3864

Merged
merged 12 commits into from
Jan 4, 2024

Conversation

joyfulmantis
Copy link
Collaborator

@joyfulmantis joyfulmantis commented Nov 11, 2023

This PR rewrites the way we check if a plugin is enabled for a request. This allows us to give more in-depth logging when we don't have any plugins that accept a request.

Things left to do for this PR

  • Documentation
  • Ensure edge cases work well

@joyfulmantis joyfulmantis changed the title Draft: Bounce completion requests and improve no plugin messages Bounce completion requests and improve no plugin messages Nov 15, 2023
@joyfulmantis joyfulmantis marked this pull request as ready for review November 15, 2023 10:44
@fendor fendor requested review from michaelpj and fendor December 2, 2023 10:31
@fendor fendor added the status: needs review This PR is ready for review label Dec 2, 2023
@michaelpj
Copy link
Collaborator

Okay, just getting to this. I'm unsure about the bouncing, tbh. I think it's probably just going to mask failures, which might be bad. What did you think about my suggestion here? I think we could do something similar for completions: use a key that's something like just the fully qualified name or something. Even if we had to do a mixed strategy (e.g. use the fully qualified name for non-local names, use a unique key for local names), that still means that we'd get resolve hits for e.g. all non-local names even if things are a bit stale.

Could we do the plugin error bit separately while we think about it?

@joyfulmantis
Copy link
Collaborator Author

Okay, just getting to this. I'm unsure about the bouncing, tbh. I think it's probably just going to mask failures, which might be bad. What did you think about my suggestion here? I think we could do something similar for completions: use a key that's something like just the fully qualified name or something. Even if we had to do a mixed strategy (e.g. use the fully qualified name for non-local names, use a unique key for local names), that still means that we'd get resolve hits for e.g. all non-local names even if things are a bit stale.

Could we do the plugin error bit separately while we think about it?

Okay sounds good, I'll dump the bounce stuff from this PR then, and we can revisit it later if we still need it.

@joyfulmantis joyfulmantis changed the title Bounce completion requests and improve no plugin messages Improve no plugin messages Dec 28, 2023
@joyfulmantis joyfulmantis requested a review from 0rphee as a code owner December 28, 2023 15:04
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

I like the general direction, a few thoughts

ghcide/src/Development/IDE/Plugin/HLS.hs Outdated Show resolved Hide resolved
hls-plugin-api/src/Ide/Types.hs Outdated Show resolved Hide resolved
hls-plugin-api/src/Ide/Types.hs Outdated Show resolved Hide resolved
hls-plugin-api/src/Ide/Types.hs Outdated Show resolved Hide resolved
hls-plugin-api/src/Ide/Types.hs Outdated Show resolved Hide resolved
hls-plugin-api/src/Ide/Types.hs Outdated Show resolved Hide resolved
plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

This looks great now

ghcide/src/Development/IDE/Plugin/HLS.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM!

ghcide/src/Development/IDE/Plugin/HLS.hs Outdated Show resolved Hide resolved
hls-plugin-api/src/Ide/Types.hs Show resolved Hide resolved
@joyfulmantis joyfulmantis merged commit e5f6931 into haskell:master Jan 4, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs review This PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants