Fix ${config:a.b} https://github.com/Microsoft/vscode-cpptools/issues…#2166
Conversation
| let keys: string[] = name.split('.'); | ||
| keys.forEach((key: string) => { config = (config) ? config.get(key) : config; }); | ||
| newValue = (config) ? config.toString() : undefined; | ||
| newValue = (config) ? config.get(name).toString() : undefined; |
There was a problem hiding this comment.
I'm not sure about this change. Basically you're removing a line of code that looks like it's trying to change config for a reason. However, based on my limited knowledge of this, I'm not sure what the point would be. The problem is that the code is so specific, that it seems questionable to change it so dramatically.
There was a problem hiding this comment.
This seems backwards to me. The previous case first did the iteration over the key array and threw an exception (the bug we're trying to fix). So, we should try that first and if we throw, then we fall back to the direct setting.
There was a problem hiding this comment.
It doesn't make sense to try the code that we expect to fail and have no repro which actually works. I think the VS Code API changed. I just added that try/catch in case there's some reason the single "get" might fail that we're not aware of. I'd prefer to avoid throwing an exception in the common/expected code path.
There was a problem hiding this comment.
But what this means is that if there IS any behavior that we haven't been able to discover, then that behavior will break. If we do the opposite, that won't occur. It is a good point that we shouldn't regularly throw in a common code-path, so is there a way to do the same thing without relying on an exception?
There was a problem hiding this comment.
The old code no longer matches the VS Code APIs used so it doesn't make sense to fall back to it. Either it never worked or the API/types changed.
3 participants
Fixes #2165 .