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

Create .gitignore before trigger build #232

Merged
merged 1 commit into from
May 9, 2020

Conversation

wingyplus
Copy link
Contributor

Create a .gitignore file by using project_dir in server state before
trigger build to make sure it create in the right place.

Close #206

:ok <- File.write(gitignore_path, "*", [:read]) do
state
else
_ -> state
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what should we do when have a problems while creating a file.

Copy link
Member

Choose a reason for hiding this comment

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

I think just logging a warning is good (using JsonRpc.log_message(:warning, message))

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 taking this on! It is on the right track but I have a few comments.

:ok <- File.write(gitignore_path, "*", [:read]) do
state
else
_ -> state
Copy link
Member

Choose a reason for hiding this comment

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

I think just logging a warning is good (using JsonRpc.log_message(:warning, message))

defp create_gitignore(%{project_dir: project_dir} = state) do
with gitignore_path <- Path.join([project_dir, ".elixir_ls", ".gitignore"]),
:ok <- gitignore_path |> Path.dirname() |> File.mkdir_p(),
:ok <- File.write(gitignore_path, "*", [:read]) do
Copy link
Member

Choose a reason for hiding this comment

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

Why use :read here? Wouldn't :write make more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. This should be :write

@wingyplus
Copy link
Contributor Author

@axelson your suggestion was addressed. please kindly review.

@wingyplus
Copy link
Contributor Author

I see test failed in dialyzer. Not sure why it happens.

https://travis-ci.org/github/elixir-lsp/elixir-ls/jobs/684546804

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.

Just one small change

state
else
true ->
JsonRpc.log_message(:warning, ".elixir_ls/.gitignore already exists")
Copy link
Member

Choose a reason for hiding this comment

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

Since this is an expected case, we don't need this log message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@axelson fixed.

@axelson
Copy link
Member

axelson commented May 8, 2020

I'm pretty sure that dialyzer error is spurious and unrelated.

Create a .gitignore file by using project_dir in server state before
trigger build to make sure it create in the right place.

Close elixir-lsp#206
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 sticking with this (and thanks to @mikl for the original idea!). I think it'll be very helpful for people. ❤️

@axelson axelson merged commit bef542e into elixir-lsp:master May 9, 2020
@mikl
Copy link

mikl commented May 9, 2020

@wingyplus yeah, thanks for taking the time to do this, it’ll be one less thing newcomers will have to deal with.

axelson added a commit that referenced this pull request May 9, 2020
@wingyplus
Copy link
Contributor Author

Thanks for you both. I learned a lot from this 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.

Idea: put a .gitignore in the .elixir_ls folder
3 participants