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

Improve session folder handling and added events for handling them #544

Merged
merged 2 commits into from
Dec 20, 2018

Conversation

yyoncho
Copy link
Member

@yyoncho yyoncho commented Dec 14, 2018

Fixes #512

  • added lsp-auto-require-clients to remove the need to require lsp-clients.
    When set to t(default) it will autorequire lsp-clients.

  • lsp command ignores the clients which binary is not present.

  • lsp no longer throws an error when there is no client installed

  • lsp will ask which client to power on if there are multiple clients for a
    single mode present(also if their binary is available). This will happen each
    time you open a new file. In order to disable that you will have to remove the
    server that you do not want to run from lsp-clients. Later, when we solve
    Can't configure lsp servers with file/directory local variables #405 we will be able to specify per project preferences like: ProjectA uses
    ccls, projectB uses clangd.

  • If you want your client to run in parallel with other server you will have to
    specify :add-on? = t when registering the client.

  • added 3 different message lsp--info, lsp--warn and lsp--error

  • cleanup

  • fixed support for tcp LANGUAGE server which apparently works only on Linux.

(lsp-register-client
 (make-lsp-client :new-connection (lsp-tcp-connection
                                   (lambda (port)
                                     `("php"
                                       ,(expand-file-name "~/.composer/vendor/felixfbecker/language-server/bin/php-language-server.php")
                                       ,(format "--tcp-server=localhost:%s" port)
                                       "--memory-limit=4095M")))
                  :major-modes '(php-mode)
                  :server-id 'php-ls))

@MaskRay
Copy link
Member

MaskRay commented Dec 15, 2018

I don't recommend advertising (add-hook 'prog-mode-hook 'lsp). It can be a supported configuration though. People don't use LSP for every prog-mode derivative.

lsp.el Outdated Show resolved Hide resolved
@yyoncho
Copy link
Member Author

yyoncho commented Dec 15, 2018

I don't recommend advertising (add-hook 'prog-mode-hook 'lsp). It can be a supported configuration though. People don't use LSP for every prog-mode derivative.

After this PR when you open a file which does not have corresponding language server it will print the following message(no errors):

selection_068

I will extend the readme section with:
"You could either attach lsp to prog-mode-hook which will try to start a language server for all programming languages and log a message when there is no configuration for the current major-mode or you could try to start lsp only for specific modes(+example code). "

What do you think?

@yyoncho
Copy link
Member Author

yyoncho commented Dec 15, 2018

More precisely:

"You could either attach lsp to prog-mode-hook which will try to start a language server for all programming languages and log a message when there is no configuration for the current major-mode and the corresponding language server is installed or you could try to start lsp only for specific modes(+example code). "

README.org Outdated
Add the following line in your configuration file.
#+BEGIN_SRC emacs-lisp
(require 'lsp)
(add-hook 'prog-mode-hook 'lsp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even a warning language server ... is not available for some mode may be annoying for some people.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option will be for initial configuration and targeted at beginners which are looking for zero configuration. Once you find the message irritating you can change it to work as you want(e. g. attach to the proper mode hook). Of course we may restructure the README based on the user feedback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beginners may not know what prog-mode-hook is and this suggestion does not provide any convenience.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People choose language servers very conservatively, as the status quo tools are not necessarily worse than LSP counterparts.

README.org Outdated
Add ~lsp~ server call to ~hack-local-variables-hook~ which runs right after the local variables are loaded.
#+BEGIN_SRC emacs-lisp
(add-hook 'hack-local-variables-hook
(lambda () (when (derived-mode-p 'prog-mode) (lsp))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prog-mode example should be specific as I have mentioned that prog-mode might not be a good example...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above.

lsp.el Outdated
(defun lsp-message (format &rest args)
"Wrapper over `message' which preserves the `eldoc-message'.
FORMAT with ARGS are the original message formats."
(-let ((inhibit-message lsp-inhibit-message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does plain let work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will change it.

(DEPRECATED_send-sync nil :read-only t)
;; ‘add-on?’ when set to t this will indicate that the server can be started as
;; a additional to another server. In case there are several
(add-on? nil :read-only t)
Copy link
Member

@MaskRay MaskRay Dec 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter name and its associate comments are still obscure to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, the comment is not finished. When the server has add-on? = true this means that it will be started along the other servers for that mode. For example, spring-boot-language-server could run in parallel with jdt-ls. I am open for suggestions for better naming.

(put 'lsp-note 'flymake-category 'flymake-note)
(put 'lsp-warning 'flymake-category 'flymake-warning)
(put 'lsp-error 'flymake-category 'flymake-error)
;; (put 'lsp-note 'flymake-category 'flymake-note)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but it does not work as I mentioned on Gitter - the Flymake modeline is displaying [0 0] with this change. I guess it is either a Flymake bug or Flymake misconfiguration but we need to figure it out before re-enabling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will recompile latest emacs and I will test on emacs 26.1

lsp.el Outdated
@@ -1071,6 +1092,10 @@ If NO-WAIT is non-nil, don't synchronously wait for a response."
(cl-defun lsp-request (method params &key no-wait)
(lsp--send-request `(:jsonrpc "2.0" :method ,method :params ,params) no-wait))

(defun lsp-request-async (method params callback &optional mode)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mode may be a good candidate for a keyword parameter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will be updated.

lsp.el Outdated
lsp--eldoc-saved-hover-message nil)

(let (signature-response hover-response)
(if (lsp--capability "signatureHelpProvider")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By splitting signatureHelp and hover, the user can use lsp-eldoc-hook to do:

  • ignore signatureHelp
  • use signatureHelp only in evil-insert-state
  • other fancy stuff

That was why I added the hook.

signatureHelp may also be expensive to the server.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with the current solution is that you should select one of signature or hover, but in the general case you want to see both which does not work without lsp-ui-doc. At this point you could disable whatever method you want by altering lsp-method-requirements which will be generic way to disable certain feature, I havent yet added this to the docs VSCode works in similar way. We could also add generic method toggle feature off to make it more user friendly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But they can also provide their own callback, e.g.

(defun +my/hover-or-signature-help ()
  (if (evil-insert-state-p)
      (lsp-signature-help)
    (lsp-hover)))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change the code to support this configuration.

@yyoncho
Copy link
Member Author

yyoncho commented Dec 19, 2018

@MaskRay Pushed version based on offline discussion, please take look whether I have missed something.

- added `lsp-auto-require-clients` to remove the need to require lsp-clients.
  When set to t(default) it will autorequire lsp-clients.

- `lsp` command ignores the clients which binary is not present.

- `lsp` no longer throws an error when there is no client installed

- `lsp` will ask which client to power on if there are multiple clients for a
  single mode present(also if their binary is available). This will happen each
  time you open a new file. In order to disable that you will have to remove the
  server that you do not want to run from `lsp-clients`. Later, when we solve
  emacs-lsp#405 we will be able to specify per project preferences like: ProjectA uses
  ccls, projectB uses clangd.

- If you want your client to run in parallel with other server you will have to
  specify `:add-on?` = t when registering the client.

- added 3 different message `lsp--info`, `lsp--warn` and `lsp--error`

- cleanup
- fixed support for tcp LANGUAGE server which apparently works only on Linux.

``` example registration
(lsp-register-client
 (make-lsp-client :new-connection (lsp-tcp-connection
                                   (lambda (port)
                                     `("php"
                                       ,(expand-file-name "~/.composer/vendor/felixfbecker/language-server/bin/php-language-server.php")
                                       ,(format "--tcp-server=localhost:%s" port)
                                       "--memory-limit=4095M")))
                  :major-modes '(php-mode)
                  :server-id 'php-ls))
```

- Implemented signature handling in single method.

Fixes emacs-lsp#214

- Changed eldoc message to display signature when it is present.

- Replaced the message calls with lsp-message which preserves the eldoc message.
At some point we may create separate buffer which will display the
- in some cases the lightner of flymake does not display the correct count
@yyoncho yyoncho merged commit 7803af2 into emacs-lsp:master Dec 20, 2018
yyoncho added a commit to emacs-lsp/lsp-ui that referenced this pull request Dec 20, 2018
wkirschbaum pushed a commit to wkirschbaum/lsp-mode that referenced this pull request Jun 1, 2021
* use get_source_file

* fix race condition in suggest specs

Code lens request is handled asynchronously. If the file is closed and close notification handle_info executes before suggest_contracts handle_call the previous code added an entry to awaiting_contracts. That entry used URI of a closed file that would later lead to crash when handling analysis_ready
Fixes elixir-lsp/vscode-elixir-ls#186
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Automatic client detection
2 participants