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

Include all core Elixir apps in PLT #225

Merged
merged 1 commit into from
Apr 28, 2020
Merged

Include all core Elixir apps in PLT #225

merged 1 commit into from
Apr 28, 2020

Conversation

ericentin
Copy link
Contributor

Seems like all the apps from the core Elixir distribution should be in the PLT?

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 change! ❤️

@axelson axelson merged commit 3d337ae into elixir-lsp:master Apr 28, 2020
axelson added a commit that referenced this pull request Apr 28, 2020
@lukaszsamson
Copy link
Collaborator

Shouldn't core erlang apps like :kernel, :compiler and :stdlib be included as well? Also I'm not sure if :iex is rely needed.

@ericentin
Copy link
Contributor Author

Shouldn't core erlang apps like :kernel, :compiler and :stdlib be included as well? Also I'm not sure if :iex is rely needed.

It seems to me that the core erlang stuff is already included by dialyzer itself, but maybe not? :iex does have an API though (https://hexdocs.pm/iex) and can be called by code that is analyzed by the language server.

@lukaszsamson
Copy link
Collaborator

I'm not sure about that given the recent elixir PR https://github.com/elixir-lang/elixir/pull/9975/files

Regarding :iex and other optional apps, it probably won't hurt but IMO if an app uses other app's api, it should list it in the dependences. In fact elixir 1.11 will emit a warning in that case.

@axelson
Copy link
Member

axelson commented Apr 29, 2020

Removing :iex is a good point. Although it seems like we should we just match the updated list from that PR.

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