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

Trim final newlines #76

Merged
merged 1 commit into from
Jun 10, 2020
Merged

Trim final newlines #76

merged 1 commit into from
Jun 10, 2020

Conversation

J3RN
Copy link
Contributor

@J3RN J3RN commented Apr 17, 2020

Remove all trailing newlines after the final newline in the file.

This actually does not contradict "files.insertFinalNewline" as I feared it would, as the documenation reads

When enabled, will trim all new lines after the final new line at the end of the file when saving it.

(emphasis mine)

This actually does not contradict `"files.insertFinalNewline"` as I feared it would. The documenation reads
> When enabled, will trim all new lines _after the final new line_ at the end of the file when saving it.

(emphasis mine)
@jayjun
Copy link
Contributor

jayjun commented Jun 8, 2020

I agree with adding this to html-eex.

But I wonder if all formatting settings in elixir can be replaced by

{
  "editor.formatOnSave": true
}

@axelson
Copy link
Member

axelson commented Jun 8, 2020

Hmm, I think this setting would still be useful since some people may turn off format on save. Although I think it would be best to not make format on save the default until the ElixirLS formatter is improved. In particular elixir-lsp/elixir-ls#289 should be fixed, and probably elixir-lsp/elixir-ls#252 as well.

@J3RN
Copy link
Contributor Author

J3RN commented Jun 8, 2020

formatOnSave seems like the sort of thing I would leave as opt-in. My current team does not use the Elixir formatter at all, though we might be more of an exception than a rule.

trimFinalNewlines, conversely, is a good practice for most files, Elixir and otherwise, IMO. However, it would be far out of scope for us to enforce that rule for all files 😄

@jayjun
Copy link
Contributor

jayjun commented Jun 8, 2020

Cool, I agree. Looks good to me.

@J3RN
Copy link
Contributor Author

J3RN commented Jun 9, 2020

@axelson Is this good to merge?

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.

Yeah it is! Thanks! ❤️

@axelson axelson merged commit 13df6fb into elixir-lsp:master Jun 10, 2020
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