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

Purge consolidated protocols before compilation #406

Merged
merged 4 commits into from Nov 21, 2020

Conversation

lukaszsamson
Copy link
Collaborator

@lukaszsamson lukaszsamson commented Nov 18, 2020

I was able to isolate this to a test case. It turns out that this is only evident with dialyzer on. The issue is triggered by module load in


As pointed out earlier this needs a refactor (#407)

I applied a solution similar to phoenixframework/phoenix@b5580e9

Fixes #395

Enum.map(beams, &(&1 |> Path.rootname(".beam") |> String.to_atom() |> purge_module()))
end

# Code.delete_path(path)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to delete the path here? It seems like it is because we don't use Code.prepend_path, so I guess the question is what purpose is Code.prepend_path serving in the phoenix code reloader. I don't feel like I personally understand why Code.prepend_path is needed in the Phoenix case, so I'm not sure if we would need it or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know that either but here Code.delete_path(path) doesn't seem to make any difference. BTW after compile it is prepended again.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, in that case maybe we just leave a comment explaining the reasoning.

@axelson
Copy link
Member

axelson commented Nov 19, 2020

For future reference, here is the ecto issue that initiated the discussion: elixir-ecto/ecto#3464

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.

Looks good! ❤️

@axelson axelson merged commit 32b6226 into elixir-lsp:master Nov 21, 2020
@lukaszsamson lukaszsamson deleted the ls-fix-consolidation-warnings branch November 21, 2020 13:44
@dvic
Copy link

dvic commented Jan 26, 2021

any chance this could be released :)?

@axelson
Copy link
Member

axelson commented Jan 26, 2021

Yeah, good idea. I'll plan to cut a release this weekend.

@axelson
Copy link
Member

axelson commented Feb 1, 2021

@dvic FYI, just released 0.6.3 which includes this fix 👍

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.

Protocol has already been consolidated warnings
3 participants