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

find elixir modules that require adding an alias #155

Merged

Conversation

ajayvigneshk
Copy link
Contributor

@ajayvigneshk ajayvigneshk commented May 9, 2022

Basic code to find modules that require adding an alias. Sort of related to elixir-ls issue
Marking it draft due to some known test failures that I would like to get some suggestions on how to handle them.

@ajayvigneshk
Copy link
Contributor Author

ajayvigneshk commented May 9, 2022

Two of the tests fail as the newly added code suggests Kernel.SpecialForms as a new alias. I'm thinking this can be ignored from suggestions to fix.
One other test failure can be fixed by ignoring the suggestions from the new code(I've used this approach in some tests already, not sure if it's okay though)

I've updated code to be behind an opt

@ajayvigneshk ajayvigneshk force-pushed the find_elixir_mod_requiring_alias branch from 2cc2520 to f0efccc Compare July 14, 2022 02:15
@ajayvigneshk ajayvigneshk marked this pull request as ready for review July 14, 2022 02:20
@ajayvigneshk ajayvigneshk force-pushed the find_elixir_mod_requiring_alias branch 2 times, most recently from a3e66a9 to ac18bb9 Compare August 27, 2022 11:21
case mod_info do
%State.ModFunInfo{positions: [{line, column}]} ->
# Hacky :shrug
{line + 1, column - 10 + 2}

Choose a reason for hiding this comment

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

What do these numbers represent?

I think you could extract them to variables, just to get the magic-number look out of it:

Suggested change
{line + 1, column - 10 + 2}
line_offset = 1
column_offset = -8
{line + line_offset, column + column_offset}

lib/elixir_sense.ex Outdated Show resolved Hide resolved
@ajayvigneshk
Copy link
Contributor Author

@antedeguemon @lukaszsamson I've applied the suggestions and removed the usage of then due to failures for v 1.11.

@ajayvigneshk
Copy link
Contributor Author

ajayvigneshk commented Oct 5, 2022

@antedeguemon @lukaszsamson Made a commit to fix credo errors

@ajayvigneshk ajayvigneshk requested review from lukaszsamson and antedeguemon and removed request for lukaszsamson and antedeguemon October 7, 2022 13:36
@lukaszsamson lukaszsamson merged commit de43440 into elixir-lsp:master Oct 17, 2022
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.

3 participants