-
Notifications
You must be signed in to change notification settings - Fork 121
Add workspaceSymbols.includeCommentSections
setting
#8419
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
Add workspaceSymbols.includeCommentSections
setting
#8419
Conversation
E2E Tests 🚀 |
The special header comments will still show up in the outline by default, right? This only changes the command palette workspace symbols? |
Right, workspace symbols are different from outline symbols, and this only affects the quick pick for workspace symbols ( |
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.
"default": false, | ||
"description": "%r.configuration.symbols.includeAssignmentsInBlocks.description%" | ||
}, | ||
"positron.r.workspaceSymbols.includeCommentSections": { |
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.
So all of these settings that we have here in this extension that include positron
in their names need to be changed, since all settings are Positron settings. What will need to happen is that we:
- deprecate the original setting using
deprecationMessage
: https://code.visualstudio.com/api/references/contribution-points#Configuration-property-schema - migrate the setting, similar to what we've done here with "prereleases"
Can we avoid adding more to this future task by changing the setting string? I would suggest ark.workspaceSymbols.includeCommentSections
since all the smarts are coming from ark.
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.
The ark.
prefix sounds good. But do we really want to spread the settings across two different sections in the Settings page in the interim?
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.
Worth noting that these ark settings will probably have to migrate to a new namespace again within the next year when the LSP becomes independent of the kernel.
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.
We chatted elsewhere and cannot come up with a good alternative that will keep us from needing to migrate in the future. So we'll just take our lumps here, and sign up for more future pain. 😭
Branched from #8412
For #4886
Extension side of posit-dev/ark#866
Can be merged separately.