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

fix: include new plugin behaviour in detection list #296

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zachdaniel
Copy link
Contributor

The plugin module was renamed from ElixirSense.Plugin to ElixirLS.LanguageServer.Plugin, to ElixirSense.Providers.Plugin (both renames broke spark's plugin, not a big deal but if you can try not to rename those modules w/o letting me know so I can get ahead of it w/ a release of spark, that would be great.)

However, the new behaviour module is not in the list used to detect a plugin, so I have to use a behaviour that doesn't exist.

Additionally, module attributes from .module_info that are persisted appear (or can appear) as lists, and so we need to update the way we check for those flags.

This stuff has a lot of cruft, and I fully understand if we need to change it in the future, and I'm happy to participate in whatever changes we want to make. I know that there are likely changes coming to language servers soon, my only request is that I'm given a chance to justify the plugin logic's existence. It truly is a huge quality of life improvement for Ash users :)

zachdaniel added a commit to ash-project/spark that referenced this pull request Sep 14, 2024
fix: support latest elixir_ls changes

unfortunately, we're going to have a failing CI  until elixir-lsp/elixir_sense#296
is merged
@lukaszsamson
Copy link
Collaborator

While I'm OK with merging that, for the future we need to think about better separating elixir_sense and the new language server from plugins. Currently when elixir_sense loads ash plugins that depend on a behaviour defined in elixir_sense it usually leads to broken server state. Loading the plugin purges and reloads elixir_sense modules from a different version and after that all request fail due to missing struct fields or functions. I've seen countless crash logs related to ash. I think we need to design a stable interface and make it a separate library.

@zachdaniel
Copy link
Contributor Author

I'm curious why loading the plugin does that? It would make sense for it to do that while I'm developing on spark because we have a dev dependency on elixir_sense so we can test. But for other users, who don't have a dev dependency on elixir_sense, why would loading the Spark plugin cause that behavior?

Regardless, I completely agree that this should be rethought. I don't know about separating it from the language server entirely, though. Especially given that the new LS would be "the Elixir standard LS", meaning that whatever it defines is the interface for a plugin would be the only interface for a language server plugin that matters.

@zachdaniel
Copy link
Contributor Author

Is it possible that the logs you see are just from when I'm working on spark and so have a dev dependency on elixir_sense that differs from what the LS has? We also (a very very long time ago) instructed users to add a dev dependency on elixir_sense. That is not necessary any more. So perhaps those users could also be generating crashes in your logs?

@lukaszsamson
Copy link
Collaborator

So perhaps those users could also be generating crashes in your logs?

It's possible.

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.

2 participants