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

Implemented multiple folders support #409

Merged
merged 2 commits into from
Oct 9, 2018

Conversation

yyoncho
Copy link
Member

@yyoncho yyoncho commented Aug 23, 2018

Fixes #345

  • Added methods for adding/removing folders
  • Tested with JDT server which seems to be the only server supporting the
    feature.
  • Verified that clients which does not support workspace folders work as
    expected(verfied with lsp-python)

@vibhavp
Copy link
Member

vibhavp commented Aug 29, 2018

Thanks for the PR! Unfortunately, I've been a little busy with real life, so it might take some time before I get to review this PR.

@yyoncho
Copy link
Member Author

yyoncho commented Aug 29, 2018

@vibhavp sure, I will extend the PR description with more information on how it is going to work to help you with the reviewing.

@yyoncho
Copy link
Member Author

yyoncho commented Sep 1, 2018

Updated the fix - added some more explanation in the commit description. Fixed corner cases related to deletion of the last element. I have also renamed the function lsp--get-project-root to lsp--suggest-project-root because the name should not imply that the method returns the proper project root.

@yyoncho
Copy link
Member Author

yyoncho commented Sep 10, 2018

@vibhavp did you had a chance to take a look?

Fixes emacs-lsp#345

* Changed the startup code of the lsp-workspace. Now it will check whether there
is file in the workspace root which contains workspace folders. If there is such
file it will be used as a source for workspace folders. If there is no such file
it will work as before that change.
* Added methods for adding/removing folders
* Tested with JDT server which seems to be the only server supporting the
feature.
* Verified that client which does not support workspace folders work as
expected(verfied with lsp-python)
@yyoncho
Copy link
Member Author

yyoncho commented Sep 15, 2018

Rebased onto master and removed formatting changes to avoid constant merging.

@yyoncho
Copy link
Member Author

yyoncho commented Sep 28, 2018

@MaskRay - can you take a look, it looks like @vibhavp is pretty busy these days?

lsp-methods.el Outdated Show resolved Hide resolved
;; language extension to keep backward compatibility with `lsp-java'
workspace-folders (append
(lsp--read-from-file
(concat (file-name-as-directory root) ".folders"))
Copy link
Member

Choose a reason for hiding this comment

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

Is it conventional to use $root/.folders?

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 am open to alternative suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

@yyoncho You might think I can merge PRs in this repo but I can't.. The most I can give you is an approval..

lsp-methods.el Outdated Show resolved Hide resolved
@MaskRay
Copy link
Member

MaskRay commented Sep 29, 2018

Is #356 superseded by this PR? You may also ask @samrayleung to review

I don't know/write much elisp...

@ramsayleung
Copy link

Is #356 superseded by this PR? You may also ask @samrayleung to review
I don't know/write much elisp...

To be honest, so do I....

@stardiviner
Copy link

I have an question, does this PR add a feature that user can get lsp-mode works without lsp--cur-workspace? For example, I want to get code completion with company-lsp in Org Mode source code opened dedicated buffer.

@yyoncho
Copy link
Member Author

yyoncho commented Oct 2, 2018

@stardiviner no. Can you share which languages do you want to use in org-mode buffers?

@stardiviner
Copy link

@yyoncho lsp-python and lsp-php and lsp-java.

@yyoncho
Copy link
Member Author

yyoncho commented Oct 4, 2018

@stardiviner do you know that should be done on org-mode side to do the integration? I am looking at https://stackoverflow.com/questions/14704077/how-to-enable-auto-complete-in-emacs-org-babel is this how it is supposed to work?

@stardiviner
Copy link

Org Mode seems can't implement this. The answers does not work in this case. The first selected answer use narrow to create a new indirect buffer that can change major-mode. But will mess up original buffer syntax highlighting (font-locking?). The second answer need the Org source block language support session. Also this session support are usually in REPL buffer.

If lsp-mode can't support completion without workspace. I think my feature request is not able to implemented.

Sorry for distribution.

@yyoncho
Copy link
Member Author

yyoncho commented Oct 5, 2018

@stardiviner I believe that it can work by providing session properties, but I need working example to start with and I can provide a sample which works with LSP mode.

@MaskRay
Copy link
Member

MaskRay commented Oct 9, 2018

For C/C++ lsp-mode users who want to try out this PR, I have implemented workspace folders in ccls in the weekend.

I'm down for this PR as long as there is still some way to enable automatic root detection which is convenient for browsing random new projects.

@yyoncho
Copy link
Member Author

yyoncho commented Oct 9, 2018

@MaskRay can you elaborate? This CL does not break the current behaviour(ability to browse random new projects), it only adds an option to call add/remove folders for a particular workspace.

@inspell
Copy link

inspell commented Oct 9, 2018

This issue is PITA for lsp-java. Could someone from lsp-mode take a look?

@vibhavp
Copy link
Member

vibhavp commented Oct 9, 2018

LGTM, Apologies for the late review. Thanks!

@vibhavp vibhavp merged commit e1bbfb5 into emacs-lsp:master Oct 9, 2018
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.

6 participants