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

Insert final newline depending on lsp4j formatting settings #649

Merged
merged 1 commit into from
Apr 28, 2020
Merged

Conversation

xorye
Copy link

@xorye xorye commented Apr 21, 2020

First step to fix redhat-developer/vscode-xml#196

Signed-off-by: David Kwon [email protected]

@xorye
Copy link
Author

xorye commented Apr 22, 2020

In my latest commit, the code now trims the final newline only if the lsp4j trimFinalNewlines option is true. With vscode-xml, the option is currently null, so FormattingOptions#isTrimFinalNewlines returns false.

Some tests are failing right now, partly because the current code does not trim final spaces. (I think the code should always trim final spaces? If yes, I will go ahead and implement that)

For example, if _ were spaces:
before formatting

<a           ></a>_____

after formatting

<a></a>

@xorye
Copy link
Author

xorye commented Apr 22, 2020

And for cases where FormattingOptions#isTrimFinalNewlines is false, I think we should make sure to only trim final spaces, not final newlines.

Like so:

before formatting

<a      ></a>
__________\n
____\n
_____\n

before formatting

<a></a>
\n
\n
\n

@xorye
Copy link
Author

xorye commented Apr 23, 2020

The latest commit makes the formatting settings/behaviour be more consistent with HTML's

HTML's "preserve new lines" settings preserve newlines in between tags, therefore does not affect the newlines at the end of the document whatsoever. This PR implements that behaviour, the xml.format.preservedNewlines preference now only applies to newlines in between tags.

In HTML, there is no HTML setting to keep more than 1 newline at the end of the document after formatting. Either there are no newlines, or there are only one.
This can be configured using the html.format.endWithNewline setting.

To have this in vscode-xml, we should introduce a xml.format.endWithNewline preference, and maybe have the setting be passed into the formatting request, so that it is accessible from the lsp4j FormattingOptions.

So as it stands right now, to keep consistency with HTML, I think it makes sense to always trim final lines at the end, and only add the newline if xml.format.endWithNewline is true.

All of this is (with the exception of the vscode-xml preference, this would be handled in a PR in vscode-xml) implemented in the latest commit of this PR

@xorye
Copy link
Author

xorye commented Apr 27, 2020

With this vscode-xml PR: redhat-developer/vscode-xml#248 the files.trimFinalNewlines and files.insertFinalNewline vscode settings get mapped to xml.format.trimFinalNewlines and xml.format.insertFinalNewline respectively.

Because of this line: https://github.com/xorye/lsp4xml/blob/ea1669063effe5d842abcc11405b4613354c7179/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/settings/XMLFormattingOptions.java#L354

If the formatting options from the formatting request has the insertFinalNewlineor trimFinalNewlines setting, they will override xml.format.insertFinalNewline/xml.format.trimFinalLines.

@@ -168,7 +168,7 @@ public void formatSettings() {
formattingOptions.setTabSize(5);
formattingOptions.setInsertSpaces(false);

XMLFormattingOptions xmlFormattingOptions = new XMLFormattingOptions(formattingOptions);
XMLFormattingOptions xmlFormattingOptions = new XMLFormattingOptions(formattingOptions, false);
Copy link
Author

@xorye xorye Apr 28, 2020

Choose a reason for hiding this comment

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

I did this because since initializeDefaultSettings calls super.setTabSize(DEFAULT_TAB_SIZE);. When merging formattingOptions, the tabSize will not get updated because it has already been set: https://github.com/eclipse/lemminx/blob/55e92f3c0f51717787f2c3b161181dc2654c293d/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/settings/XMLFormattingOptions.java#L346-L356

@angelozerr angelozerr merged commit f12cf5d into eclipse-lemminx:master Apr 28, 2020
@angelozerr
Copy link
Contributor

Great job @xorye ! Could you add a section which explains those 2 settings (bound with vscode-xml) in https://github.com/redhat-developer/vscode-xml/wiki/Formatting please

@xorye xorye added this to the 0.12.0 milestone Jun 10, 2020
@xorye xorye added the enhancement New feature or request label Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formatting of newlines at EOF
3 participants