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

Make code_reload act regardless of diagnostics #1423

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

fabjan
Copy link
Contributor

@fabjan fabjan commented Mar 4, 2023

Description

See #1409 for background. Given that hot-reloading from an editor is a development feature it should not matter if there are diagnostic errors while iterating.

The reason I moved the code out from els_compiler_diagnostics is that it seemed a bit misplaced in there and looking at commit history it looks like code reload was added before the els_diagnostics behavior existed, and it was just inside els_text_synchronization_provider. If it is to be detached from diagnostics results it seems there is little reason to keep it in els_compiler_diagnostics.

Fixes #1409

Extras

Stop trying to compile and reload files that are not ".erl" files. It doesn't work and just gives popup spam.

Skärmavbild 2023-03-05 kl  08 59 08

Testing

The moved test cases work fine. I also manually tested that code reloading still works.

Notes

If the remote node has a cookie it is not possible to configure code reload to work with it. Perhaps the "code_reload" section in the config file needs to get a "cookie" attribute (but for a future PR I guess).

@dweinstein
Copy link

I tend to agree with @fabjan here. It's weird to get spurious errors like "code_reload swap crashed for: erlang_ls.config with: error" when opening/saving the erlang_ls.config file or *.app.src files.

I'd say either this change or an option in erlang_ls.config so that if I hit save on a file it tries to reload regardless of anything else such as warnings. I've found that it's quite confusing that, e.g., I have this issue #1044 and due to that it wouldn't code_reload my cowboy handler.

image

@dweinstein
Copy link

separately @fabjan

If the remote node has a cookie it is not possible to configure code reload to work with it. Perhaps the "code_reload" section in the config file needs to get a "cookie" attribute (but for a future PR I guess).

putting the cookie into the erlang_ls.config doesn't sound like a good idea since that would be committed? Seems like something to pick up from another file or environment variable instead?

@fabjan
Copy link
Contributor Author

fabjan commented Mar 17, 2023

putting the cookie into the erlang_ls.config doesn't sound like a good idea since that would be committed? Seems like something to pick up from another file or environment variable instead?

That sounds like a better idea. However, there is already a cookie setting in the config, so I figured this might have been considered already:

-spec get_cookie() -> atom().

Maybe the scope for the runtime config is very specific and "LS-only", so to speak?

@plux plux merged commit 7163398 into erlang-ls:main Dec 21, 2023
@plux
Copy link
Contributor

plux commented Dec 21, 2023

Thanks for your contribution @fabjan !

shuying2244 pushed a commit to shuying2244/erlang_ls that referenced this pull request Dec 30, 2023
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.

code reload should be triggered despite compilation warnings
3 participants