-
Notifications
You must be signed in to change notification settings - Fork 196
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
Replace unknown remote function #776
Replace unknown remote function #776
Conversation
@lukaszsamson thanks for the update! So I will wait with contributing until #773 is merged. |
@scohen That's great! I will definitely come back to this feature soon |
7e9109e
to
e01f381
Compare
@scohen I've updated that so now it should fit the new way of doing code actions |
{:., meta1, [{:__aliases__, meta2, ^module}, ^name]} -> | ||
{:., meta1, [{:__aliases__, meta2, module}, suggestion]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: meta1
and meta2
are not descriptive names, one is the function call meta and the other is the module meta. Suggest function_meta
and module_meta
apps/language_server/lib/language_server/experimental/code_mod/replace_remote_function.ex
Outdated
Show resolved
Hide resolved
apps/language_server/lib/language_server/experimental/provider/handlers/code_action.ex
Outdated
Show resolved
Hide resolved
@pattern ~r/(.*)\/(.*) is undefined or private. .*:\n(.*)/s | ||
|
||
@spec pattern() :: Regex.t() | ||
def pattern, do: @pattern |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this exposes private implementation details, I have an example of how to handle this without doing so below.
|> Enum.map(&String.to_atom/1) | ||
|> List.pop_at(-1) | ||
|
||
{module, name} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, module isn't actually a module, it's a list, and it will be missing the leading Elixir
that all elixir modules have.
...uage_server/lib/language_server/experimental/provider/code_action/replace_remote_function.ex
Outdated
Show resolved
Hide resolved
...uage_server/lib/language_server/experimental/provider/code_action/replace_remote_function.ex
Outdated
Show resolved
Hide resolved
def apply(%CodeAction{} = code_action) do | ||
source_file = code_action.source_file | ||
diagnostics = get_in(code_action, [:context, :diagnostics]) || [] | ||
@pattern ~r/variable "([^"]+)" is unused/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Pattern is a much less descriptive name than variable_re
, but we should not be exposing this anyways.
apps/language_server/test/experimental/provider/code_action/replace_with_underscore_test.exs
Outdated
Show resolved
Hide resolved
apps/language_server/test/experimental/provider/code_action/replace_with_underscore_test.exs
Outdated
Show resolved
Hide resolved
...uage_server/lib/language_server/experimental/provider/code_action/replace_remote_function.ex
Outdated
Show resolved
Hide resolved
...uage_server/lib/language_server/experimental/provider/code_action/replace_remote_function.ex
Outdated
Show resolved
Hide resolved
def apply(source_file, diagnostic) do | ||
with {:ok, module, name} <- extract_function(diagnostic.message), | ||
{:ok, suggestions} <- extract_suggestions(diagnostic.message), | ||
one_based_line = extract_line(diagnostic), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm matching against the Types.Diagnostic module in the extract_line
It's better to do it inapply
, you wrote a typespec, so why not:
def apply(%SourceFile{} = source_file, %Diagnostic{} = diagnostic) do
...
end
it seems like none of the keys in diagnostic.range.start.line have an optional type, so why isn't it guaranteed that one_based_line is an integer
Because elixir is a dynamic language and there are no types.
What I'm saying is: move one_based_line out of the with statement.
module_list | ||
|> Enum.map(fn module -> | ||
module | ||
|> Atom.to_string() | ||
|> String.trim_leading("Elixir.") | ||
end) | ||
|> Enum.join(".") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then this is just
module_string = Enum.map_join(module_list, ".", &Atom.to_string/1)
apps/language_server/test/experimental/provider/code_action/replace_with_underscore_test.exs
Outdated
Show resolved
Hide resolved
apps/language_server/test/experimental/provider/code_action/replace_with_underscore_test.exs
Outdated
Show resolved
Hide resolved
I've found out that replacing wasn't working if the module of the function to replace was aliased. Fixed in the second commit (9a01a18). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more comment regarding the tests, and we're good to merge
apps/language_server/test/experimental/provider/code_action/replace_remote_function_test.exs
Outdated
Show resolved
Hide resolved
Enum.concat(a) | ||
end | ||
end | ||
] |> Document.new() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, this works very well; I think the structural == will even work with documents. Very nice.
I've prepared that PR to propose extending code actions by replacing unknown remote functions. For example, the following code:
contains an unknown function:
counts/1
. After this change, we would get 2 possible Quick Fixes:that rewrite our code to:
or
respectively.
In our example, Quick Fixes were generated from the following Elixir warning:
I find such a feature very convenient, so I'm waiting for comments and feedback.