-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: Make documentation on hover configurable #9264
Conversation
crates/rust-analyzer/src/config.rs
Outdated
/// Whether to show documentation on hover. | ||
hover_documentation: bool = "true", | ||
/// Use markdown syntax for links in hover. | ||
hover_linksInHover: bool = "true", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR splits up hoverActions
from general hover configurations as I feel like a split makes more sense here. This does move hoverActions_linksInHover
to hover_linksInHover
however which is a breaking change. I suppose we can keep both settings and mark the old version deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented configuration deprecation now 178b5ff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, why do we need deprecation functionality? We already have aliases (see behaviour vs behavior), and all the aliases but one are effectively deprecated -- they don't show up in the scheme. If you want to do anything else for deprecated aliases, I think you can build off that functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right I guess we don't need that then? For some reason I thought aliases only worked for the last part of config names. I'll change that back then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason I thought aliases only worked for the last part of config names.
They might, I dont' remember. But, even that's the case, we should extend that functionality, rather than add a new, similar one. Might be a good idea to document this!
pub markdown: bool, | ||
pub documentation: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like these two fields want to be something like
pub docs: Option<DocFormat>
enum DocFormat {
Markdown, PlainText
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as far as the feature goes, but I think hover module can benefit from some cleanup before tackling more functionality onto it!
I'll clean this up in a follow up then as this is big enough as is |
bors r- |
Canceled. |
Just to double check, are you sure we need deprecated functionality here? #9264 (comment) |
Oh sorry I didnt see that actually(github could really improve on showing whats new in a PR) |
bors r+ |
Build failed: |
bors r+ |
This also implements deprecation support for config options as this renames
hoverActions_linksInHover
tohover_linksInHover
.Fixes #9232