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

WIP: Configuration file support #338

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

Conversation

axelson
Copy link
Member

@axelson axelson commented Aug 4, 2020

Add a user configuration file that is read on startup.
Change the server settings to always be fully fleshed out. So the
ElixirLS.LanguageServer.Server state settings always has all the
settings (and as atoms) instead of being an empty map by default. This
consolidates the default setting logic into the new ConfigParser
module instead of being strewn about the Server.

TODO:

  • Fix tests (I'm getting a strange error with the tests that I cannot figure out.)
  • Read a configuratioon file from the repository

Note: Editor configuration always overrides the configuration file, and
the VSCode extension always sends in the full configuration. This means
that the configuration file is not currently useable with the VSCode
extension. This will probably have to be modified in the VSCode
extension because I think it is important for the extension to have the
final control of the server settings.

Fixes #60

Add a user configuration file that is read on startup.
Change the server settings to always be fully fleshed out. So the
ElixirLS.LanguageServer.Server state `settings` always has all the
settings (and as atoms) instead of being an empty map by default. This
consolidates the default setting logic into the new `ConfigParser`
module instead of being strewn about the Server.

TODO:
- [ ] Fix tests (I'm getting a strange error with the tests that I cannot figure out.)
- [ ] Read a configuratioon file from the repository

Note: Editor configuration always overrides the configuration file, and
the VSCode extension always sends in the full configuration. This means
that the configuration file is not currently useable with the VSCode
extension. This will probably have to be modified in the VSCode
extension because I think it is important for the extension to have the
final control of the server settings.
alias ElixirLS.Utils.ConfigParser
alias ElixirLS.LanguageServer.JsonRpc

# Can we change this to load also from the workspace root? Note: might need multiple passes for that to work
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to remove this comment before merging

|> String.split(["\n", "\r", "\r\n"], trim: true)
|> Enum.map(&String.trim/1)
# Ignore json comments
|> Enum.reject(&String.starts_with?(&1, "#"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

haha, good catch. I've obviously haven't been doing very much js development recently. However since we're not using a full processor I don't think it's feasible to support multi line comments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. A simple full line comment support is good enough.

@@ -0,0 +1,7 @@
# This is the configuration file for ElixirLS
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment should be // as @lukaszsamson comment above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, fixed!

@@ -0,0 +1,7 @@
# This is the configuration file for ElixirLS
{
# Disable dialyzer
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be // also.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

@lukaszsamson
Copy link
Collaborator

@axelson how far is this from being done?

@axelson
Copy link
Member Author

axelson commented Oct 7, 2022

@lukaszsamson Hmmm, I think it was fairly close at the time, but I think it's pretty out of date now so I don't know how much work would be required to bring it up to date. I also probably won't be able to dedicate time to it in the near future.

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.

Add support for a configuration file
3 participants