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

Add ALERename (tsserver & LSP), ALEOrganizeImports (tsserver) and auto import support (tsserver) #2709

Merged
merged 56 commits into from
Sep 12, 2019

Conversation

jeremija
Copy link
Contributor

@jeremija jeremija commented Aug 17, 2019

Related to #1519

ALERename [new_name]

Tested for tsserver, and should work for any LSP that has rename capability, but I currently do not have a LSP project to test it on. Currently only supports renaming of symbols (no file rename).

ALEOrganizeImports

Works with tsserver and will organize all import statements when called.

Auto Imports

Currently works with tsserver. The ts@completions message now includes the parameter: includeExternalModuleExports. If an external completion is returned, the necessary code actions will be stored in the user_data variable of the completion item.

To support both ale-manual and ale-automatic completion triggers, a new augroup ALECompletionActions has been added.

This functionality can be disabled by setting g:ale_completion_tsserver_autoimport to 0.


A new function was added for handling code actions: ale#code_action#HandleCodeAction. I've tested this extensively on a TypeScript project. I would appreciate feedback, especially on the implementation of this function. There might be a better way to edit the buffer without resorting to visual mode.

mwilliammyers and others added 27 commits February 16, 2019 15:29
Add protocol support for the LSP and tsserver
Still need to implement the actual renaming in vim
Also replace \n in replacement with <CR>
@w0rp w0rp added this to In Review in On the Radar via automation Aug 17, 2019
@w0rp
Copy link
Member

w0rp commented Aug 17, 2019

Nice work! 👍 I've got a long list of emails to get through, and I'll take a look when I can.

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

Great work! This kind of thing is difficult to manage. See my suggestions here. I have some of my own code for this kind of stuff already which you can use, plus some stuff to come later, when I finally have the time.

autoload/ale/lsp/tsserver_message.vim Outdated Show resolved Hide resolved
autoload/ale/lsp/tsserver_message.vim Outdated Show resolved Hide resolved
autoload/ale/rename.vim Outdated Show resolved Hide resolved
autoload/ale/code_action.vim Outdated Show resolved Hide resolved
\}

if has_key(l:suggestion, 'codeActions')
let l:result.user_data = json_encode({
Copy link
Member

Choose a reason for hiding this comment

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

Try removing json_encode here and json_decode below. I think you can store Dictionary values too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so too, but I get E731: using Dictionary as a String when I remove the json_encode/json_decode functions :(

autoload/ale/organize_imports.vim Outdated Show resolved Hide resolved
doc/ale.txt Outdated Show resolved Hide resolved
doc/ale.txt Show resolved Hide resolved

if !empty(l:lsp_linters)
let l:prompt = 'new name: '
let l:new_name = a:0 ? join(a:000) : input(l:prompt, expand('<cWORD>'))
Copy link
Member

Choose a reason for hiding this comment

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

Instead of supporting an optional argument for a new name in the command, always require the new name to be input with input() and the current name. Then you always know exactly what name ALE is going to try and rename, and it's easy to adjust the current name to something slightly different. See here: ca2026b#diff-c8e42b11378165eaf5fb1be0e53860e6R90

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented this change but since feedkeys() doesn't work for input prompt, I added a new util method ale#util#Input(message, value) and mocked that method in the test. Let me know if this works!

autoload/ale/rename.vim Outdated Show resolved Hide resolved
Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

Thank you very much for working on this, and sorry for the delay. This is great work. When I finally get the time, I'll build on this later by finishing my work on showing code actions in menus, including the popup menu.

@w0rp
Copy link
Member

w0rp commented Sep 12, 2019

Let's get people to try this out for a while, and we can fix any bugs people find later. Cheers! 🍻

@w0rp w0rp merged commit 3e8c8d3 into dense-analysis:master Sep 12, 2019
On the Radar automation moved this from In Review to Done Sep 12, 2019
@jeremija
Copy link
Contributor Author

Thanks for your guidance and thank you for merging! 🍺

@cirias
Copy link

cirias commented Sep 17, 2019

Nice work! But I found autoimport doesn't work when I use deoplete. Is it possible to support that?

@jeremija
Copy link
Contributor Author

Hi @cirias, I do not use deoplete so I can't test this. Could you try to add a check for deoplete completion source on this line:

if l:source isnot# 'ale-automatic' && l:source isnot# 'ale-manual'

it should look like:

    if l:source isnot# 'ale-automatic' && l:source isnot# 'ale-manual' && l:source isnot# 'deoplete'

Let me know if this works!

@cirias
Copy link

cirias commented Sep 17, 2019

I tried. But it still doesn't work. Also I found the problem is, when g:ale_completion_enabled is 0, it doesn't list any external symbols as candidates. As long as let g:ale_completion_enabled = 1, it works correct. Hope that helps you debug

@jeremija
Copy link
Contributor Author

jeremija commented Sep 17, 2019

Perhaps we should move this conversation to a new issue. I just installed and configured deoplete to use ale and I believe I was able to reproduce the issue.

Can you confirm that the same happens to you:

a) deoplete is configured to use ale
b) external symbols show up the first time I enter insert mode and start typing. auto import works for external symbols listed if I add the change from my comment above
c) external symbols stop showing up the next time I enter insert mode

edit I believe the reason I'm experiencing this is because Deoplete caches completion results, and ale reduces the number of completion results to 50. When a user types S in a Typescript file, there will many completion results. Values such as SyntaxError (global) and SyntaxKind (from typescript) will be filtered out, given to Deoplete, and Deoplete will cache them, so when Syn is typed, neither will be shown.

I was able to resolve this by setting self.is_volatile = True in the Base.__init__ in rplugin/python/deoplete/sources/ale.py file.

From deoplete docs:

is_volatile
		(Bool)				(Optional)
		If it is True, the source depends on the user input. It means
		that if this flag is set to False, deoplete will cache
		gather_candidates results and will not call gather_candidates
		on each input change. Only on_post_filter method will be
		called on each input change (if implemented).

		Default: False

On a side note, I noticed that the ale#completion#GetCompletionResult function frequently gets called many, many (too many?) times from ale.py.

@w0rp
Copy link
Member

w0rp commented Sep 18, 2019

Importing external symbols isn't currently designed to work with Deoplete. That will require some additional work. Open an issue for that.

@thisjeremiah
Copy link

thisjeremiah commented Sep 19, 2019

Excited that this feature could replace our usage of nvim-typescript!

I have noticed that the autoimport feature does not work when autocomplete is manually executed via <c-o><c-x>. Is that the intended behavior?

@jeremija
Copy link
Contributor Author

@thisjeremiah What configuration are you using to trigger completion via <c-o><c-x>?

This is my configuration for <C-Space> and it works for me:

imap <C-Space> <Plug>(ale_complete)

@thisjeremiah
Copy link

I've actually just been using completion via <enter> or <esc>, which successfully fills out the term selected in the popup menu, but the autoimport does not execute.

I set up your mapping and it worked, probably because it is calling ale_complete directly. I'll probably use that for now. Thanks!

In general, I'm trying to reduce the steps needed to autoimport an already written term. The :TSImport command from nvim-typescript had a few less steps since that command could be run from normal mode with the term selected, whereas this command has to run in insert mode and manually execute ale_complete.

@heyrict
Copy link

heyrict commented Sep 19, 2019

Glad that ale is on the road to support CodeAction! I used to depend heavily on YCM's FixIt command when developing typescript projects and this will finally remove YCM from my YCM + ALE stack!

One thing (though not bug) I found inconvenient is that, You don't know which file the symbol you are importing from. e.g. When I want to import compose from react-redux, it shows compose from several other packages which are hard to differenciate.

Screenshot from 2019-09-19 19-34-33

@w0rp
Copy link
Member

w0rp commented Sep 19, 2019

If anyone knows enough about Deoplete to make it work, please create a pull request.

I plan on adding code action support with Vim menus later, when I finally have the time. I have some experimental code for that in a branch.

I did notice one error with renaming in other files I saw once, but I haven't been able to repeat it since.

@jeremija
Copy link
Contributor Author

I believe I found a fix for Deoplete, see #2779. @cirias can you test this?

@jeremija
Copy link
Contributor Author

@heyrict The autoimport description should now be visible on master since 7b38e97 commit. See #2780 for more details.

timlag1305 pushed a commit to timlag1305/ale that referenced this pull request Nov 5, 2019
…o import support (tsserver) (dense-analysis#2709)

This commit adds support for renaming symbols in tsserver and with LSP tools, and for organising imports with tsserver. Completion results for symbols that can be imported are now suggested if enabled for tsserver completion done via ALE.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants