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

Ajust suggestions/autocomplete for multiple clients/scenarios #300

Merged
merged 1 commit into from
Jun 21, 2020

Conversation

msaraiva
Copy link
Collaborator

This PR addresses some of the problems related to suggestions/autocomplete I could find when working/simulating the UX in different editors.

Context

This resumes the of the work done in #273 and also addresses #281.

Making everything work nicely for every situation turned out to be much more complicated than I expected. The reason is that we have many variables to take into account, for instance:

  • is the function configured to have parenthesis? (according to the .formatter)
  • is signature supported?
  • is snippet supported?
  • is there a capture before?
  • is there a pipe before?
  • is there any text after? what kind of text?
  • does the function has any argument? if so, is there any default argument?
  • are there other functions with the same name but different arity?

Any different answer to any of the questions above can lead to a different behaviour in any of the following features:

  • List of suggestions
    • which functions with the same name should be excluded?
  • Signature hint
    • when to show?
    • at what argument?
  • Auto-completion/snippets (different rules may apply to different functions in the same list)
    • Insert just the name
    • Insert name + arity
    • Insert name + arguments (with or without parenthesis)
    • Insert name + arguments (with or without parenthesis), removing unused default arguments

Examples

Regarding #281. The detail now looks pretty much like the TypeScript (and other languages) version, which was presented by @lukaszsamson in this comment.

image

With the details box hidden:

image

Functions with default arguments, which generates multiple functions, might be listed differently depending on the context. For instance, if signature support is enabled, only the original function is listed since all the other derived function also have the same docs, spec and snippet:

image

After confirmation:

image

However, there are situations where we want the list all functions. Here a couple of examples:

If you have a capture prefix

It will list all derived functions since each of them will have a different snippet:

image

After confirmation:

image

Functions configured to not include parenthesis or when using editors without signature support

image

In this case, the arguments added will correspond to the arity selected.

Selecting add/2:

image

Selecting add/3:

image


There are many other rules implemented for many of the other cases I mentioned. Feel free to give it try and please let me know if you find any issue or have any question.

It's going to be really hard to have a solution that can please everyone. The intention is to improve UX for most users without compromising usability for the others.

@msaraiva
Copy link
Collaborator Author

@axelson feel free to include other people that might also be interested in this PR.

/cc @dgutov @joaotavora

@dgutov
Copy link
Contributor

dgutov commented Jun 19, 2020

@msaraiva Looking good on my end. Overall, it feels like a return to the previous behavior, but with some finer touches. Thanks!

Functions configured to not include parenthesis...

Looking at the code, it seems to be a part of configuration probably more visibly exposed in the VS Code plugin.

But should it just contain unqualified names? I could imagine preferring to have them qualified. Or even being able to list whole modules at once (such as Ecto.Migration).

With the details box hidden

This is an aside, but how does one do that in VS Code? Is there a key binding?

@msaraiva
Copy link
Collaborator Author

Hi @dgutov! Thanks for reviewing it. ❤️

Looking at the code, it seems to be a part of configuration probably more visibly exposed in the VS Code plugin.

The configuration mentioned refers to the :locals_without_parens option that can be set (or imported) in .formatter so it will apply to any editor.

But should it just contain unqualified names? I could imagine preferring to have them qualified. Or even being able to list whole modules at once (such as Ecto.Migration).

I'm not sure a understand what you mean. Sorry. Can you elaborate more on that?

This is an aside, but how does one do that in VS Code? Is there a key binding?

Hitting ctrl + space toggles the details box.

@dgutov
Copy link
Contributor

dgutov commented Jun 19, 2020

Hitting ctrl + space toggles the details box.

Thanks!

The configuration mentioned refers to the :locals_without_parens option that can be set (or imported) in .formatter so it will apply to any editor.

Could you point to some documentation and/or example for that file, so I can better answer the question?

@msaraiva
Copy link
Collaborator Author

msaraiva commented Jun 19, 2020

Could you point to some documentation and/or example for that file, so I can better answer the question?

Sure. Here are the docs and an example (ecto_sql).

@dgutov
Copy link
Contributor

dgutov commented Jun 19, 2020

Sure. Here are the docs and an example (ecto_sql).

Thank you. So I guess it's some existing standard in use by other tools (formatter?), which makes it infeasible to change it, and my suggestion off topic. Sorry :-)

But what I was saying that instead of

locals_without_parens: [some_dsl_call: 2, some_dsl_call: 3]

it would be better to specify module names explicitly

locals_without_parens: ["Module.Name": [some_dsl_call: 2], "Another.Module": :*]

or something along these lines. I suppose the formatter didn't have the ability to analyze the code deep enough to recognize where each function was exported from when this was introduced.

@msaraiva
Copy link
Collaborator Author

I suppose the formatter didn't have the ability to analyze the code deep enough to recognize where each function was exported from when this was introduced.

Yup. The formatter has no knowledge about imported nor aliased functions and probably never will. It just formats the code based on those pre-defined rules.

@msaraiva
Copy link
Collaborator Author

@axelson I believe we're ready to merge this. Please, let know if you think it deserves any further discussion or testing.

BTW, I'm not sure it would be feasible or not but it would be nice if we could have a small group of people containing at least one user/maintainer of each different editor/plugin that could validate changes like this one. Since most of us are usually using a single editor, it's hard to know for sure how a specific change might impact all clients.

Copy link
Member

@axelson axelson left a comment

Choose a reason for hiding this comment

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

Thanks for this! It looks great and I like all of the attention to detail about completion in different contexts! ❤️

BTW, I'm not sure it would be feasible or not but it would be nice if we could have a small group of people containing at least one user/maintainer of each different editor/plugin that could validate changes like this one. Since most of us are usually using a single editor, it's hard to know for sure how a specific change might impact all clients.

Yes I think that would be good. What would the main editor/plugin combos be? Here's my first stab at a list:

  • Emacs/lsp-mode
  • Emacs/eglot
  • VSCode
  • Vim/coc.vim
  • Vim/vim-lsp

@lud
Copy link

lud commented Mar 26, 2022

Hello, I am not sure if it is related to this issue, but I found that the parentheses are skipped even when the call is not local.

I have a module with a field/3 function that I use quite a lot, and I always use it prefixed with the module name, as in MyMod.field. But elixirls does autocompletion with a space instead of a parenthese.

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.

5 participants