Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Handle multiple compile commands on client side (needs native server side support) #12960
base: main
Are you sure you want to change the base?
Handle multiple compile commands on client side (needs native server side support) #12960
Changes from 7 commits
f346723
2dc0fa3
02a2812
280aee1
b934a92
e273731
0c8ccb1
8e84183
22de1e0
62a8915
d7e2a4a
b1614de
7fe9904
82c4e53
69c4013
d2bf51d
6aa66f6
45bb041
fbe7a2b
2438391
dc3e543
82d5c0b
c7a9c05
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 description should be updated
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.
updated the description, feel free to suggest something different, English is not my native language.
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.
Question for my team. Do we want to prefer storing the compileCommands property as a string in the actual
c_cpp_properties.json
file, or as an array? Either way is acceptable here because the json will be reparsed inhandleConfigurationChange
and converted back to an array.One other thing to note is that we should change the Edit Configurations UI to make the
input
for compileCommands be atextarea
instead, along with whatever associated changes should come with that. The answer to the question above would also guide the implementation for the storage of the data in the json file. (e.g. if we prefer string for the single entry case, we'd need to test for that when it comes via the UI and react accordingly)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.
Quick suggestion for your first question, might be an overkill. Instead of forcing the value to be a string array both in
parsePropertiesFile()
and inhandleSquiggles()
, we create a "version 5" of the properties and add a function "updateToVersion5()" which actually saves a single entry as an array in the json file.Again, this might be overkill, just a suggestion.
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 have considered making changes to the version number, but this gets disruptive because many people commit their configurations to source control. We received a lot of negative feedback in the past when version changes were more frequent so we avoid it as best we can.
We are technically breaking versioning and assuming some risk by not incrementing the version number with this change, but the addition is very subtle. We will need to discuss this as a team and decide if this is acceptable or not.
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.
@bobbrow Can't it be a string or an array in the c_cpp_properties.json?
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.
Should this thread be resolved now?
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.
minor: I don't believe you need to use
?.
here or on the line below because the existence ofconfiguration.compileCommands
was tested before entering this code block.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.
fixed
Check failure on line 1156 in Extension/src/LanguageServer/configurations.ts
GitHub Actions / job / build
Check failure on line 1156 in Extension/src/LanguageServer/configurations.ts
GitHub Actions / job / build
Check failure on line 1156 in Extension/src/LanguageServer/configurations.ts
GitHub Actions / job / build
Check failure on line 416 in Extension/src/LanguageServer/settings.ts
GitHub Actions / job / build
Check failure on line 416 in Extension/src/LanguageServer/settings.ts
GitHub Actions / job / build
Check failure on line 416 in Extension/src/LanguageServer/settings.ts
GitHub Actions / job / build
Check failure on line 424 in Extension/src/LanguageServer/settings.ts
GitHub Actions / job / build
Check failure on line 424 in Extension/src/LanguageServer/settings.ts
GitHub Actions / job / build
Check failure on line 424 in Extension/src/LanguageServer/settings.ts
GitHub Actions / job / build
Check failure on line 425 in Extension/src/LanguageServer/settings.ts
GitHub Actions / job / build
Check failure on line 425 in Extension/src/LanguageServer/settings.ts
GitHub Actions / job / build
Check failure on line 425 in Extension/src/LanguageServer/settings.ts
GitHub Actions / job / build