nixd/Server/configuration: use .nixd.json for workspace/configuration defaults
#261
Conversation
There was a problem hiding this comment.
Thanks for your contribution!
Left some comment that I think it is unnecessary to change public codes, and also, the commit message should be revised as path/to/subsystem: change, with more details about the diff (don't say fix bug, instead, say what you have been fixed).
| readJSONConfig(); | ||
| configuration::TopLevel JSONConfigCopy = JSONConfig; | ||
| updateConfig(std::move(JSONConfigCopy)); | ||
| WorkspaceConfiguration = |
There was a problem hiding this comment.
Don't know why we need to move this statement here? It is just an assignment to WorkspaceConfiguration.
There was a problem hiding this comment.
I have probably overestimated how concurrent the code is, but the reasoning was that the callback to initialized could run before the assignment to WorkspaceConfiguration, making the resulting fetchConfig call fail.
There was a problem hiding this comment.
@inclyc Could you confirm whether or not this could happen?
| llvm::StringRef PayloadKind) { | ||
| T Result; | ||
| llvm::StringRef PayloadKind, T Base = T()) { | ||
| T Result = Base; |
There was a problem hiding this comment.
This function is very fundamental to the LSP library, I think we'd better do such thing in Controller.cpp, not public codes (here).
There was a problem hiding this comment.
I would argue that it's reasonable to have an optional argument that defines the defaults for parsing. It is an optional parameter so it shouldn't change the behaviour of the function (although maybe move semantics could be used to make it more efficient).
If I can't change this then I will either have to copy the implementation of this function into Controller.cpp (breaks DRY) or make a more complicated system of defaulting that acts on the parsed configuration::TopLevel structs (more error-prone and less efficient). Either of these options would make the code less maintainable. What would you suggest?
There was a problem hiding this comment.
structs (more error-prone and less efficient).
How about use a dedicated function like updateConfig and do the work there? I think introduce a new default argument in the basic library is more error prone for large C++ project (since it might implicitly change many things)
There was a problem hiding this comment.
I have refactored parseParam again so that it no longer takes a default argument and behaves the same way as before (uninitializing Result by default or using the default constructor if this is not possible, as well as not compiling if neither is possible). However, it now shares code with parseParamWithDefault, which has an argument that determines some default JSON that should be used. I think that this is a reasonable function to add to a general-purpose LSP library.
inclyc
changed the title
.nixd.json for workspace/configuration defaults
|
I will rebase and squash the commits on my end as they aren't currently intended to be looked at individually. |
jonathanjameswatson
force-pushed
the
update-config-with-json
branch
from
54517c5 to
3ddcdf3
Compare
…on` defaults Add jjw-log for options.enable Actually use BaseConstructor Fix some bugs Fix move bug Remove jjw-log Format changed files
jonathanjameswatson
force-pushed
the
update-config-with-json
branch
from
3ddcdf3 to
ae7f63f
Compare
| @@ -122,6 +128,7 @@ class Controller : public lspserver::LSPServer { | |||
|
|
|||
| std::mutex ConfigLock; | |||
|
|
|||
| configuration::TopLevel JSONConfig; | |||
There was a problem hiding this comment.
maybe you can add some comment here, saying that it is the default value of config
There was a problem hiding this comment.
On second thoughts, I think I should call it DefaultConfig and then add a comment that says it is set to the contents of .nixd.json if it exists.
When
nixdinworkspace/configurationis unset in VSCode, we get[null]. This causes the JSON configuration to be used (not overwritten). However, in Emacs, we get[{}]instead, so the JSON configuration is overwritten with the default one. By using the JSON configuration as the defaults forworkspace/configuration, we get the same behaviour whenworkspace/configurationis unset in VSCode and Emacs. The JSON configuration is no longer ignored whenworkspace/configurationis set, but it never takes priority.An additional advantage of this change is that when
workspace/configurationis unset (or set tonull) while the language server is running, we fall back to the JSON configuration instead of keeping the old deletedworkspace/configuration.Feel free to make any improvements to my branch.