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

Split "clangd.arguments" setting into multiple options #50

Open
i-ky opened this issue Jul 6, 2020 · 3 comments · May be fixed by #122
Open

Split "clangd.arguments" setting into multiple options #50

i-ky opened this issue Jul 6, 2020 · 3 comments · May be fixed by #122
Labels
enhancement New feature or request

Comments

@i-ky
Copy link

i-ky commented Jul 6, 2020

I have to admit that it is a very future-proof design, no matter what argument may be introduced in clangd in the future, we have a way to pass it from clangd extension. However, at the same time this approach isn't the most user friendly, because user has no hints when filling array of options via UI or directly in JSON file.

@i-ky i-ky added the enhancement New feature or request label Jul 6, 2020
@reporter123
Copy link

I vote for still keep clangd.arguments for options not provided by other means.

@rapgenic
Copy link
Contributor

I started looking on this and a small inconsistency appeared: for simplicity I assumed that the explicit single options should have precedence over the manually specified settings if they are present both ways.

This way if those options are present in the arguments setting too, they are replaced by the UI defined value.

However this might be problematic when updating the extension to a new version with this feature: if a user had their own options already defined in the arguments setting and they were different from the defaults defined in the new version, they would be replaced, possibly causing confusion to the user.

Personally I see three solutions:

  • Allowing the new settings to be null in addition to their real value and defaulting their value to null. This way we can check if they're defined in the first place and only if so override the arguments setting. However this could cause a lot more confusion especially with boolean values, because it would be very difficult to understand which option sets what value.
  • Parsing the current arguments variable and extracting the new options value from it. Problematic to handle with mixed global and workspace settings (which setting should we parse?) and probably overcomplicated.
  • Giving up on trying to fix the conflicts and letting the user fix them on his own, maybe showing a popup warning if such conflicts are detected. Cons: not very user-friendly and popups tend to clutter the user experience

Also what about older clangd versions that may not like certain options? Should we assume that a recent version should be used?

@rapgenic
Copy link
Contributor

An alternative I did not consider is giving the arguments setting precedence (maybe documenting it on the configuration help). This way it should not break current configurations and is probably the simplest and cleanest implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants