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

glennsarti · 2018-07-12T11:50:04Z

PR Summary

Previously the code folding feature would fail to find region blocks starting
with Region or ending with EndRegion. This was due to the regex only
looking for lower case. This commit updates the start and end region
detection to be case insensitive, similar to that defined in
microsoft/vscode@64186b0

This commit adds has strict whitespace rules for the region/endregion keyword
which means only #region will match whereas # region will not.

Previously the code folding had trouble detecting the beginning of comment
blocks and regions which were not indented. This was due to the empty line
regex expecting at least one whitespace character to be considered "an empty
line". This commit modifies the empty line regex to also allow an empty string
which is indeed, an empty line.

Fixes #1428

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

PR has a meaningful title
Summarized changes
This PR is ready to merge and is not work in progress
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

rkeithhill · 2018-07-12T14:37:38Z

src/features/Folding.ts

@@ -354,7 +354,7 @@ export class FoldingProvider implements vscode.FoldingRangeProvider {
    ): ILineNumberRangeList {
        const result = [];

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


Does JS really require the [] here? Wouldn't /^\s*$/ be sufficient?

Huh....Good point. Will test

rkeithhill · 2018-07-12T14:38:10Z

src/features/Folding.ts

-        const emptyLine = /^[\s]+$/;
-        const startRegionText = /^#\s*region\b/;
-        const endRegionText = /^#\s*endregion\b/;
+        const emptyLine = /^[\s]*$/;


Same comment as above.

rkeithhill · 2018-07-12T14:40:20Z

src/features/Folding.ts

-        const startRegionText = /^#\s*region\b/;
-        const endRegionText = /^#\s*endregion\b/;
+        const emptyLine = /^[\s]*$/;
+        const startRegionText = /^#\s*[rR]egion\b/;


Does JS support (?i) to set the regex to ignore case?

Depends. This commit brings it into line with the VSCode "default" behaviour, but, we could add this. I'm not fussed either way on this issue.

Good enough for me. FWIW, ISE only supports lower-case #region / #endregion.

OTOH leading folks down a path that doesn't work in ISE … is that a bad thing and do we care? Also, ISE doesn't like a space between the # and region|endregion. It's pretty picky.

My inclination would be:

No space between # and region (similar to other pragmas, e.g. #requires, #if)

Support total case insensitivity (/^#region\b/i), since PowerShell is generally case insensitive

Interesting the tests for that, don't include spaces between #
microsoft/vscode@9e05d4b#diff-e10026493ca8c706f842f8b49b1ccdfa

I also find it odd that they allow any number of spaces after the # e.g.

# Region

would be a valid region start.

So given all this, plus the powershell region snippet has no space (

vscode-powershell/snippets/PowerShell.json

Lines 947 to 955 in 705f174

"Region Block": {

"prefix": "#region",

"body": [

"#region ${1}",

"$0",

"#endregion"

],

"description": "Region Block for organizing and folding of your code"

}

) I'll change the regex to not allow whitespace.

Can always add it back in :-)

rkeithhill

LGTM other than minor suggestions on the regex's - depending on the funky JS support for regex. :-)

glennsarti · 2018-07-13T01:22:15Z

Ready for review/merge again.

rjmholt · 2018-07-13T02:08:13Z

src/features/Folding.ts

-        const startRegionText = /^#\s*region\b/;
-        const endRegionText = /^#\s*endregion\b/;
+        const emptyLine = /^\s*$/;
+        const startRegionText = /^#\s*region\b/i;


I still think we shouldn't match # region for this; I can imagine ordinary comments matching this, and I don't think I've ever seen any usage of a pragma that permitted whitespace like this.

May want to hit up the VSCode team on that, as they're ALL like that

I can only find examples where the whitespace comes before the #, like:

The extension folding snippets

The asks in the original issue

The feature announcement

VSCode's folding tests

VSCode's test definition of FoldingMarkers

Having an indented #region like:

if ($x) { #region DoStuff #endregion }

seems pretty reasonable to me.

But I feel like setting folds for

# Region if ($x) { ... Blah } # EndRegion

could cause problems. Any line comment beginning with the word "region" would become a fold, meaning we would then look for # endregion, and may not find one.

Oh I am so sorry...I need glasses 👎

-100 points to me.

^\\s*# is not ^#\\s*

Haha, no I thought that's what was going on, just wanted to be sure 😄

Thanks so much for this @glennsarti!

…hitespace Previously the code folding feature would fail to find region blocks starting with `Region` or ending with `EndRegion`. This was due to the regex only looking for lower case. This commit updates the start and end region detection to be case insensitive, similar to that defined in microsoft/vscode@64186b0 This commit adds has strict whitespace rules for the region/endregion keyword which means only `#region` will match whereas `# region` will not. Previously the code folding had trouble detecting the beginning of comment blocks and regions which were not indented. This was due to the empty line regex expecting at least one whitespace character to be considered "an empty line". This commit modifies the empty line regex to also allow an empty string which is indeed, an empty line.

glennsarti · 2018-07-13T02:13:13Z

Pending Green CI, this is ready for review again.

rkeithhill · 2018-07-13T02:14:13Z

LGTM!

rjmholt · 2018-07-13T02:50:53Z

src/features/Folding.ts

-        const startRegionText = /^#\s*region\b/;
-        const endRegionText = /^#\s*endregion\b/;
+        const emptyLine = /^\s*$/;
+        const startRegionText = /^#region\b/i;


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

As in, like the other VSCode folding regexes

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.

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?

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.

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

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

Anyway, I'm satisfied!

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

TylerLeonhardt

LGTM 🎉

glennsarti mentioned this pull request Jul 12, 2018

PowerShell Extension 1.8.1 breaks editor folding on region tags #1428

Closed

glennsarti force-pushed the gh-1428-folding-casing branch from ae7065b to e91298b Compare July 12, 2018 12:00

glennsarti changed the title ~~(GH-1428) Make region code folding non-case sensitive parse whitespace~~ (GH-1428) Make region folding case insensitive and parse whitespace correctly Jul 12, 2018

rkeithhill reviewed Jul 12, 2018

View reviewed changes

rkeithhill approved these changes Jul 12, 2018

View reviewed changes

glennsarti force-pushed the gh-1428-folding-casing branch from e91298b to c5d3b59 Compare July 13, 2018 01:17

glennsarti force-pushed the gh-1428-folding-casing branch from c5d3b59 to e5f16ce Compare July 13, 2018 01:27

rjmholt reviewed Jul 13, 2018

View reviewed changes

glennsarti force-pushed the gh-1428-folding-casing branch from e5f16ce to c61ca6d Compare July 13, 2018 02:11

glennsarti changed the title ~~(GH-1428) Make region folding case insensitive and parse whitespace correctly~~ (GH-1428) Make region folding case insensitive and strict whitespace Jul 13, 2018

rjmholt reviewed Jul 13, 2018

View reviewed changes

rjmholt approved these changes Jul 13, 2018

View reviewed changes

rkeithhill mentioned this pull request Jul 13, 2018

(GH-1417) Fix code folding on CRLF documents #1418

Merged

3 tasks

TylerLeonhardt approved these changes Jul 16, 2018

View reviewed changes

TylerLeonhardt merged commit b4a66c3 into PowerShell:master Jul 16, 2018

	"Region Block": {
	"prefix": "#region",
	"body": [
	"#region ${1}",
	"$0",
	"#endregion"
	],
	"description": "Region Block for organizing and folding of your code"
	}

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

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

Conversation

glennsarti commented Jul 12, 2018 • edited Loading

PR Summary

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill left a comment

Choose a reason for hiding this comment

glennsarti commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti commented Jul 13, 2018 • edited Loading

rkeithhill commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

glennsarti Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

rjmholt Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt left a comment

Choose a reason for hiding this comment

glennsarti commented Jul 12, 2018 •

edited

Loading

rkeithhill Jul 12, 2018 •

edited

Loading

rkeithhill Jul 13, 2018 •

edited

Loading

glennsarti Jul 13, 2018 •

edited

Loading

glennsarti commented Jul 13, 2018 •

edited

Loading

glennsarti Jul 13, 2018 •

edited

Loading

glennsarti Jul 13, 2018 •

edited

Loading

rjmholt Jul 13, 2018 •

edited

Loading