niosus / EasyClangComplete

:boom: Robust C/C++ code completion for Sublime Text 3/4
https://niosus.github.io/EasyClangComplete/
MIT License
574 stars 79 forks source link

c_cpp_properties.json: Support ${workspaceFolder} substitution #741

Open seanmlyons22 opened 3 years ago

seanmlyons22 commented 3 years ago

c_cpp_properties.json supports vscode variables such as ${workspaceFolder}. However, this must be translated to something that LLVM can understand. I propose a small enhancement to the CCppProperties class where ECC will attempt to replace ${workspaceFolder} with . in the includePath list. According to this issue it seems that assuming that .vscode/c_cpp_properties.json is located in the root folder is a safe assumption.

I don't believe this would impact any users who do not have ${workspaceFolder} in their c_cpp_properties file.

Create minimal example if you can

An example of a json that I would like to add support for:

{
  "version": 4,
  "configurations": [
    {
      "name": "Win32",
      "includePath": [
        "${workspaceFolder}/source",
      ],
      "defines": [
      ],
    },
  ]
}

I would be willing to open a PR for this if acceptable. I tested the following change and it worked for my use case. If you're interested, I can make some tests and formalize a PR:

        def parse_includes_from_json(content):
            try:
                include_paths = content["configurations"][0]["includePath"]
            except Exception:
                include_paths = []

            # VSCode variable for current working directory
            vsc_ws = '${workspaceFolder}'
            # Attempt to replace VSCode variables if any
            includes = [i.replace(vsc_ws, '.') for i in include_paths]
            includes = [path.expandvars(i) for i in include_paths]
            includes = ["-I{}".format(include) for include in includes]
            return includes
niosus commented 3 years ago

I was not the one who implemented the support for c_cpp_properties.json, so if you think it can work better, I am happy to have a look at a PR and merge it eventually, but I will not be able to provide much guidance apart from checking how it plays together with the other parts of the code.

mclayton7 commented 3 years ago

@niosus @seanmlyons22 I think this sounds reasonable. There should be some tests that'll help verify the implementation, but if you have any questions about the existing implementation, don't hesitate to @ me!

niosus commented 3 years ago

Thanks for chiming in @mclayton7, really appreciate this! @seanmlyons22 feel free to implement what you consider makes sense, open a PR and ping both me and @mclayton7 on it. This way we can go forward with this.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Just comment here to prevent this from happening.