(GH-1417) Fix code folding on CRLF documents #1418
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously the folding provider would tokenize the entire document at a time. While this mostly works, it causes issues with regular expressions which use the `$` anchor. This anchor will interpret CRLF vs LF differently. While it may be possible to munge the document prior to tokenization, the prior art within the VS Code codebase shows that this is not the intended usage, i.e. lines should be tokenized, not an entire document. This commit changes the tokenization process to tokenize per line but still preserves the original folding behaviour.
Previously the test fixtures were failing on some Windows systems due to git changing the line endings. This commit adds a new test fixture, and runs the same tests on the same content document, but each with a different line-ending (CRLF and LF).
Previously some of the text in the test fixtures was confusing e.g. Text that says 'cannot fold' should read 'should fold'. This commit updates the text in the fixture to describe the expected behaviour for humans.
Previously only single quoted here strings were tested for folding. This commit adds a test for the double quoted herestrings as well.
Previously there was some duplication in the folder test fixtures. This commit refactors the expectation to be more DRY. This commit also adds an expectation on the line endings as we're depending on git to clone correctly. Without this we may get false positive results.
8e23f89 to
ef0e3bd
Compare
|
Ping @rkeithhill and @rjmholt |
|
Ready to throw the switch |
glennsarti
commented
Jul 11, 2018
| // Note that line endings (CRLF/LF/CR) have interpolation issues so don't | ||
| // tokenize an entire document if the line endings are variable. | ||
| const tokens: ITokenList = new Array<IToken>(); | ||
| let tokenizationState = null; |
There was a problem hiding this comment.
Should I create an interface for this...Doesn't seem like I really need to....
|
Did this break #region - #endregion folding in VSC 1.25.1? |
|
Yes it did. See #1430 for the fix - not yet merged. |
|
Thanks, I'll go back to pre-1.8.0 till the fix is merged |
|
@lucdekens You can also just disable the syntax folding so you can still get all the bugfixes etc. in 1.8.0 but not the folding.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Summary
Previously the folding provider would tokenize the entire document at a time.
While this mostly works, it causes issues with regular expressions which use the
$anchor. This anchor will interpret CRLF vs LF differently. While it may bepossible to munge the document prior to tokenization, the prior art within the
VS Code codebase shows that this is not the intended usage, i.e. lines should be
tokenized, not an entire document. This commit changes the tokenization process
to tokenize per line but still preserves the original folding behaviour.
This PR also updates the test fixtures used to be more robust and DRY.
Fixes #1417
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets.Please mark anything not applicable to this PR
NA.WIP:to the beginning of the title and remove the prefix when the PR is ready