Skip to content

Conversation

@zhengbli
Copy link
Contributor

@zhengbli zhengbli commented Mar 1, 2016

Fixes #7022 and #6700.

Review on Reviewable

@zhengbli
Copy link
Contributor Author

zhengbli commented Mar 9, 2016

pinging @vladima

@billti
Copy link
Member

billti commented Mar 9, 2016

👍 Can you port to release-1.8 also? Thanks!

zhengbli added a commit that referenced this pull request Mar 10, 2016
Avoid removing indentation on a new line as trailing white spaces
@zhengbli zhengbli merged commit e151d49 into microsoft:master Mar 10, 2016
@zhengbli zhengbli deleted the fixFormatOnEnter branch March 10, 2016 17:18
zhengbli added a commit to zhengbli/TypeScript that referenced this pull request Mar 10, 2016
zhengbli added a commit that referenced this pull request Mar 10, 2016
@mihailik
Copy link
Contributor

Shouldn't it have tests?

@zhengbli
Copy link
Contributor Author

@mihailik normally it should. Though in this case turns out it is hard to test the behavior, as VSCode indents first and then send the buffer content to the server for formatting; while VS formats first, and then ask the language service about the indentation; and our fourslash tests mimic's VS' behavior. So there is no way to know if a inserted indentation would be removed, as the test suite doesn't insert indentation. I've tested on VS and VSCode to make sure it solves the issue.

@mihailik
Copy link
Contributor

I'm not doubting it works, just for the sake of preventing regressions.

Wouldn't fourslash work for this case? I've found this example of testing indentation:

https://github.com/Microsoft/TypeScript/blob/master/tests/cases/fourslash/alignmentAfterFormattingOnMultilineExpressionAndParametersList.ts

@zhengbli
Copy link
Contributor Author

You are right verifying indentation is possible. Though the issue here is to verify the indentation after the "format on enter", which is different from formatting the whole document. For example:

function test() { // hit enter here
    | // cursor location afterwards
}

In this case, if you are in the "format on enter" case, the indentation in the current line shouldn't be removed, as you are typing. However if you are formatting the whole document, it should be removed because these are trailing whiltespaces, and formatting the whole document shouldn't be affected be cursor location. I'm saying that testing the former one is difficulty for now.

@mccreeker
Copy link

This seems really overly-complicated for determining whether a line should have an indent or the spacing be removed... I don't think other editors check with servers and go through multiple checks and edits like this.

By the way, the problem is happening in .php files with PHP or HTML code, so I'm not sure how TypeScript is involved.

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants