-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Allow merge of C_Cpp_Properties & configuration provider. #8174
Allow merge of C_Cpp_Properties & configuration provider. #8174
Conversation
Adds a configuration that enables the include paths, defines, and forced includes from `c_cpp_properties.json` to be merged with those provided by a configuration provider. This is especially helpful for projects that use unsupported compilers as defines/includePaths/forcedIncludes can be provided as shims for intellisense.
77322f5
to
c6f97a0
Compare
A step in the right direction I guess, but not helping in my use case as the cpp/cmake provider has no information (CMake could generate |
The new property See https://github.com/microsoft/vscode-cpptools/blob/main/Extension/ui/settings.html and the UI element for "see Also add this new property to https://github.com/microsoft/vscode-cpptools/blob/main/Extension/src/LanguageServer/settingsPanel.ts so that the UI will be updated with values from c_cpp_properties.json. How to localize string in HTML.
The following is an example of a span with localized content, containing a sub-node with a localized title attribute.
Child nodes are not included when content of a parent node is localized. In the outer nodes of the above example, the following string will be extracted for localization: "Switch to the {0} file by clicking on the file link or using the command:" |
Can a "C_Cpp.default.mergeConfigurations" setting be added? See https://github.com/microsoft/vscode-cpptools/pull/7845/files for an example; however, that PR has been updated to use "markdownDescription" instead of "description". |
const fileConfiguration: configs.Configuration | undefined = this.configuration.CurrentConfiguration; | ||
if (fileConfiguration?.mergeConfigurations ?? false) { | ||
configs.forEach(config => { | ||
fileConfiguration?.includePath?.forEach(p => { |
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.
Does anybody have any idea why these lint errors exist? It seems like a false positive to me but similar code in other places in this file isn't causing this error.
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.
@sean-mcmanus do you know about these lint errors?
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.
Yeah, I'm not sure why the linter is giving these errors, but they appear to be fixable via changing the forEach calls with for loops, e.g. for (const p in fileConfiguration.includePath) {
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.
Before 621ca46, I was still getting an unused expression error from the linter on line 1825 on the old side of that commit.
Seems the ?. expressions might confuse the linter?
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.
Yep, the issue seems to be eslint/eslint#12642.
Added |
const fileConfiguration: configs.Configuration | undefined = this.configuration.CurrentConfiguration; | ||
if (fileConfiguration?.mergeConfigurations ?? false) { | ||
configs.forEach(config => { | ||
fileConfiguration?.includePath?.forEach(p => { |
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.
@sean-mcmanus do you know about these lint errors?
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.
See Michelle's comment.
@willson556 these changes are now merged and will be available in 1.7.0-insiders3. Thank you for your work and contribution to the C++ extension. |
Thank you @michelleangela and @sean-mcmanus for taking the time to review and merge these changes! This is going to significantly improve our experience with IAR and VSCode! |
Partially resolves #6909 (doesn't yet work with
compile_commands.json
,just custom configuration providers).
Adds a configuration that enables the include paths, defines, and forced
includes from
c_cpp_properties.json
to be merged with those providedby a configuration provider.
This is especially helpful for projects that use unsupported compilers
as defines/includePaths/forcedIncludes can be provided as shims for
intellisense.