Skip to content

(GH-1428) Make region folding case insensitive and strict whitespace #1430

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 16, 2018
Merged

(GH-1428) Make region folding case insensitive and strict whitespace #1430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/features/Folding.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ export class FoldingProvider implements vscode.FoldingRangeProvider {
): ILineNumberRangeList {
const result = [];

const emptyLine = /^[\s]+$/;
const emptyLine = /^\s*$/;

let startLine: number = -1;
let nextLine: number = -1;
Expand Down Expand Up @@ -421,9 +421,9 @@ export class FoldingProvider implements vscode.FoldingRangeProvider {
): ITokenList {
const result = [];

const emptyLine = /^[\s]+$/;
const startRegionText = /^#\s*region\b/;
const endRegionText = /^#\s*endregion\b/;
const emptyLine = /^\s*$/;
const startRegionText = /^#region\b/i;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, I think you were right about the whitespace here -- no? That the regex should look like /^\s*#region/i?

Copy link
Contributor

Choose a reason for hiding this comment

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

As in, like the other VSCode folding regexes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope....the regex I use is slightly different, because it's checking the grammar token (it's within the line) not the whole line. That's why I do an Empty Line check.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the code below it, it looks like subsequentText() gets the whole comment line and then tests it against the regex, which starts with ^. Which I read as not matching an indented #region. Does that sound right?

Copy link
Contributor Author

@glennsarti glennsarti Jul 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So the logic I'm using is;

    Find the `#` character (This is the `punctuation.definition.comment.powershell` scope)
        If there is only whitespace before the `#` then
            if the text after, and including the `#` matches the starting regex then
                #Do Something
            end
            if the text after, and including the `#` matches the ending regex then
                #Do Something
          end
       end
     end

What this means is the start/endregion regexes will never have the whitespace in the front.

That said, it could be optimized by grabbing the whole line and regex-ing on that...but that's for another PR.

Copy link
Contributor Author

@glennsarti glennsarti Jul 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also, looks like the method subsequentText() is named wrong. Should be textFrom() or something like that.

Copy link
Contributor

@rjmholt rjmholt Jul 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, so it does this:

    #   Some text is here
    ^-------------------^
    [offset]            [end of line]

subsequentText() seems as good a name as any - but maybe the comment should be something like:

Get the text from a given offset to the end of the line it's on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, I'm satisfied!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rjmholt PR #1438 refactors the detection to be easier to understand.

const endRegionText = /^#endregion\b/i;

tokens.forEach((token) => {
if (token.scopes.indexOf("punctuation.definition.comment.powershell") !== -1) {
Expand Down
22 changes: 12 additions & 10 deletions test/features/folding.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,16 +38,18 @@ suite("Features", () => {

suite("For a single document", async () => {
const expectedFoldingRegions = [
{ start: 1, end: 6, kind: 1 },
{ start: 7, end: 51, kind: 3 },
{ start: 8, end: 13, kind: 1 },
{ start: 14, end: 17, kind: 3 },
{ start: 19, end: 22, kind: 3 },
{ start: 26, end: 28, kind: 1 },
{ start: 30, end: 40, kind: 3 },
{ start: 32, end: 36, kind: 3 },
{ start: 42, end: 44, kind: 3 },
{ start: 47, end: 50, kind: 3 },
{ start: 0, end: 4, kind: 3 },
{ start: 1, end: 3, kind: 1 },
{ start: 10, end: 15, kind: 1 },
{ start: 16, end: 60, kind: 3 },
{ start: 17, end: 22, kind: 1 },
{ start: 23, end: 26, kind: 3 },
{ start: 28, end: 31, kind: 3 },
{ start: 35, end: 37, kind: 1 },
{ start: 39, end: 49, kind: 3 },
{ start: 41, end: 45, kind: 3 },
{ start: 51, end: 53, kind: 3 },
{ start: 56, end: 59, kind: 3 },
];

test("Can detect all of the foldable regions in a document with CRLF line endings", async () => {
Expand Down
13 changes: 11 additions & 2 deletions test/fixtures/folding-crlf.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
#RegIon This should fold
<#
Nested different comment types. This should fold
#>
#EnDReGion

# region This should not fold due to whitespace
$shouldFold = $false
# endRegion
function short-func-not-fold {};
<#
.SYNOPSIS
Expand Down Expand Up @@ -30,15 +39,15 @@ double quoted herestrings should also fold

#region This fools the indentation folding.
Write-Host "Hello"
# region Nested regions should be foldable
#region Nested regions should be foldable
Write-Host "Hello"
# comment1
Write-Host "Hello"
#endregion
Write-Host "Hello"
# comment2
Write-Host "Hello"
# endregion
#endregion

$c = {
Write-Host "Script blocks should be foldable"
Expand Down
13 changes: 11 additions & 2 deletions test/fixtures/folding-lf.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
#RegIon This should fold
<#
Nested different comment types. This should fold
#>
#EnDReGion

# region This should not fold due to whitespace
$shouldFold = $false
# endRegion
function short-func-not-fold {};
<#
.SYNOPSIS
Expand Down Expand Up @@ -30,15 +39,15 @@ double quoted herestrings should also fold

#region This fools the indentation folding.
Write-Host "Hello"
# region Nested regions should be foldable
#region Nested regions should be foldable
Write-Host "Hello"
# comment1
Write-Host "Hello"
#endregion
Write-Host "Hello"
# comment2
Write-Host "Hello"
# endregion
#endregion

$c = {
Write-Host "Script blocks should be foldable"
Expand Down