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 ecto plugin #104

Closed
msaraiva opened this issue Jun 25, 2020 · 10 comments · Fixed by #105
Closed

Add ecto plugin #104

msaraiva opened this issue Jun 25, 2020 · 10 comments · Fixed by #105

Comments

@msaraiva
Copy link
Collaborator

I'm working on a plugin to improve Ecto support for editors. The first two features are:

  1. Improve snippets to provide the list of available types when completing field or add. Example:

image

  1. Context-aware suggestions for from:

image

@lukaszsamson in the example above, how hard would be to identify the variable u as an instance of %User{}?

@msaraiva
Copy link
Collaborator Author

@lukaszsamson to clarify, the question would be, can the type inference logic you're working on help here? Or do we need to do something from scratch?

@msaraiva
Copy link
Collaborator Author

msaraiva commented Jun 29, 2020

We have suggestions for query bindings :)

image

It also works with bindings defined in joins (using either var in Mod or assoc(var, ...))

image

@msaraiva
Copy link
Collaborator Author

And finally, the last feature I have for the initial version.

Suggestions for associations when using assoc/2:

image

@axelson
Copy link
Member

axelson commented Jun 30, 2020

I just want to say that having these completions be a possibility is very exciting!

@lukaszsamson
Copy link
Collaborator

Sorry for a late answer, I didn't have time to look into this earlier. This looks awesome and I guess you've already figured out the details. Anyway, here are my answers:

@lukaszsamson in the example above, how hard would be to identify the variable u as an instance of %User{}?

It depends if we are able to get the correct AST

iex(10)> quote do
...(10)> from(
...(10)> u in User,
...(10)> 
...(10)> )
...(10)> end
** (SyntaxError) iex:14: syntax error before: ')'

If we are able

iex(10)> quote do  
...(10)> from(     
...(10)> u in User 
...(10)> )
...(10)> end
{:from, [],
 [
   {:in, [context: Elixir, import: Kernel],
    [{:u, [], Elixir}, {:__aliases__, [alias: false], [:User]}]}
 ]}

then it should be easy.

@lukaszsamson to clarify, the question would be, can the type inference logic you're working on help here? Or do we need to do something from scratch?

It can be useful. I'd try to parse and transform Ecto macros into a format that Binding module can understand.
e.g.

from(
u in User,
do_sth(u)
)

into

u = %User{}
do_sth(u)

I wonder if adding support for pipe style queries, i.e.

User
|> where([u], u.

or subqueries

subquery = from(u in User)

from(s in subquery,
...

would need much work.

@msaraiva
Copy link
Collaborator Author

msaraiva commented Jul 2, 2020

Hi folks!

I'm almost done here. It took much longer than I thought. I couldn't resist and ended up implementing the ability to handle functions without parens since it's the most common way we see people using from/2. So now we can do this:

image

A great side effect of that change is that signature hint (or anything else that depends on which_func) will finally also work for functions without parens too:

image

Since we need to deal with incomplete code when autocompleting, we'll never be able to cover all possible scenarios, but I think the new implementation should cover the most common cases we have.

@lukaszsamson

I wonder if adding support for pipe style queries, i.e.

I wouldn't care much about pipe style for now, however, if we could infer types of queryables, that would be just awesome. For instance, in your example:

subquery = from(u in User)

from(s in subquery,

I'm not able to retrieve the type of subquery since we currently only retrieve types of bindings if the type is defined inside from. However, if we had the type of subquery on the buffer metadata, then I would be able to do that. I'm not sure if that would be feasible with the current implementation.

@msaraiva
Copy link
Collaborator Author

msaraiva commented Jul 5, 2020

@axelson are you ok with moving the module attribute snippets to ElixirSense? I think we shouldn't have any suggestions provided by ElixirLS itself. One great thing about the new implementation is that it allows us to provide very precise context-aware suggestions. That means we can add or remove suggestions according to the knowledge with have about the code. In the Ecto plugin, for instance, we can restrict the list to the minimum we believe that might be meaningful. For instance:

Listing available schemas in has_many and belongs_to associations:

image

Listing available options for has_many and belongs_to associations:

image

As you can see, ElixirLS adds those extra suggestions for both cases and there's nothing we can do about it from the ElixirSense side even though we know they are not applicable in the current context.

In case you agree, I can move that to ElixirSense in the context of Ecto plugin's PR.

@axelson
Copy link
Member

axelson commented Jul 5, 2020

@msaraiva I do think it makes sense in this case. Especially as ElixirLS is calling ElixirSense.suggestions/3. Although it would be good to more rigorously define the boundary between ElixirLS and ElixirSense, it is a little nebulous in my mind. My main concern as it relates to this, is what if we want to make the general completions more configurable (i.e. disable @doc false and @moduledoc false or instead disable @doc """ or @moduledoc """).

Although I do think that historically most of those completions were added to ElixirLS because jake wouldn't have been able to add them directly to ElixirSense. The current splitting of the completions feels a little awkward as a result.

@msaraiva
Copy link
Collaborator Author

msaraiva commented Jul 5, 2020

@axelson we can certainly start accepting a few configurations in ElixirSense. I think that will be important in the near future as we start to explore more context/function/argument aware suggestions. Ideally, we shouldn't probably have any suggestion nor general customization of suggestions on the ElixirLS side.

In order to have a flexible API for the plugins, I have created a new type of suggestion called :generic which is defined as:

  @type generic :: %{
    label: String.t(),
    detail: String.t() | nil,
    documentation: String.t() | nil,
    insert_text: String.t() | nil,
    snippet: String.t() | nil,
    priority: integer() | nil,
    kind: String.t()
  }

This way, when adding a different type of suggestion introduced by a plugin, we don't need to update ElixirLS to handle that suggestion. All different types of suggestions for the Ecto plugin are using :generic. In fact, I think most of the types of suggestions we have currently in ElixirLS could be replaced by the new generic type.

I'll send the PR for ElixirLS to handle the generic type along with the Ecto plugin. Then we can start incrementally to move some of the other types if we find it necessary. The only one I want to move right away is the module attribute snippets as they are the only one that does not respect the restrictions imposed by the suggestions' engine as shown in the examples above.

@axelson
Copy link
Member

axelson commented Jul 6, 2020

That sounds good to me 👍

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 a pull request may close this issue.

3 participants