[#1401] Present configuration read errors for the user #1406
Conversation
|
The notice in VS Code:
Example log message (I don't know why it shows up twice, that happens for me with any version of the language server):
|
apps/els_core/src/els_config.erl
Outdated
|
|
||
| -spec initialize(uri(), map(), map(), boolean()) -> ok. | ||
| initialize(RootUri, Capabilities, InitOptions, ReportMissingConfig) -> | ||
| -spec initialize(uri(), map(), map(), use_els_server | no_els_server) -> ok. |
There was a problem hiding this comment.
My reasoning for changing this parameter was that until I happened upon #1060 I thought the flag was only about verbosity or the config file missing. But after seeing that issue, it appears the purpose of the flag was to prevent els_server:send_notification from being called when the DAP script calls els_config:initialize/3.
(Perhaps els_config:initialize/3 can be removed completely and els_dap_general_provider.erl updated to call /4 with no_els_server).
Other names for the flag could maybe be no_notice | show_notice.
|
Feel free to close this PR if there is too much else going on. I prefer to not be the one closing it since it is not targeting my repository. |
|
LGTM, but again the CI jobs seem to be expired and could not find a way to re-run them. If you can force-push, we can merge this one. |
fabjan
force-pushed
the
better-config-read-logs
branch
from
4bfbc70
to
9fe80e4
Compare
|
I rebased on main since I was going to force push and get another CI run anyway. When doing a last minute test locally I found that a bad YAML yields a crash when merging the configs so I added a little
|
| @@ -86,33 +86,46 @@ | |||
| providers => map() | |||
| }. | |||
|
|
|||
| -type error_reporting() :: lsp_notification | log. | |||
There was a problem hiding this comment.
As I read through my changes again after some time away from it I came to the conclusion that no_els_server and use_els_server were kind of bad names for the variants. I think this is more clear.
| -spec initialize(uri(), map(), map()) -> ok. | ||
| initialize(RootUri, Capabilities, InitOptions) -> | ||
| initialize(RootUri, Capabilities, InitOptions, _ReportMissingConfig = false). | ||
| %% https://github.com/erlang-ls/erlang_ls/issues/1060 | ||
| initialize(RootUri, Capabilities, InitOptions, _ErrorReporting = log). |
There was a problem hiding this comment.
initialize/3 could be removed completely unless it is kept for some backwards compatibility I can't see.
There was a problem hiding this comment.
I don't see a reason to keep it, either.
|
@robertoaloi I really didn't mean to spam when I commented the PRs can be closed. I just wanted to make sure they were not going to linger around making a mess! I will touch them one by one now, so the other two will be "on hold" until this one is merged or closed. Sorry for scope creeping a bit here after you already reviewed it, but I just changed the names of the error reporting selector variants and a little error handling for configs that are valid YAML but not maps (and thus can't be used at all) which I discovered when picking this back up, rebasing and testing locally. |
No problem at all, thanks for the call to action and for the contribution! |
|
Thanks for the contribution! |
|
Looks like the Windows CI failed because chocolatey could not install Erlang 22.3 :( |
…ng-ls#1406) * Present configuration read errors for the user Fixes erlang-ls#1401 * Clarify error_reporting flag in els_config * Stop trying to merge config maps that are not maps
Description
If the the configuration file(s) found cannot be parsed, show a notice for the user instead of silently moving on to the next config alternative.
Fixes #1401