Add support for custom outlining regions by uniqueiniquity · Pull Request #17954 · microsoft/TypeScript

Ben Lichtman (uniqueiniquity) · 2017-08-21T22:54:11Z

This update enables support for custom outlining regions of the following form

// #region label

...code...

// #endregion

Region delimiters may not be preceded by any non-whitespace text, and they are not valid if they occur within a multiline comment.

Microsoft Contribution License Agreements (msftclas) · 2017-08-21T22:54:15Z

Ben Lichtman (@uniqueiniquity),
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot #Closed

Bill Ticehurst (billti) · 2017-08-21T23:40:57Z

+                            return regionText;
+                        }
+                    }
+                }


The logic in this whole 'if' block seems a little cumbersome. You're already using a RegExp to determine if it is a region. You should be able to use a capture group in the RegExp to extract the name also.

Changed to use capture group.

That's not the most efficient way to use capture groups. Simply do:

// Change above regexp to the below // Not removal of the gm flags and moving the left paren of the capture group past the whitespace const regionStart = new RegExp("^\\s*//\\s*#region\\s+(.*)?$"); // In here just do const result = comment.match(regionStart); return result[1] ? result[1] : regionText;

Now I've actually pushed the change. :)

Bill Ticehurst (billti) · 2017-08-21T23:46:29Z

+////*/
+////
+/////*
+////// #endregion


It would be good to have tests with unbalanced start/end markers too. This is an area where a regression could easily slip through and cause exceptions.

Added unbalanced tests.

Bill Ticehurst (billti) · 2017-08-21T23:49:31Z

+//////#endregion|]
+////
+////// Nested regions
+////[|// #region outer


Is there any way to test the name that appears for the region? (And add some tests for interesting region names, e.g. includes spaces or tabs).

As far as I can tell, fourslash doesn't currently gather that information.
I can try to add the functionality if you would like me to though.

Bill Ticehurst (billti)

Some minor changes, else looks good. Thanks!

Andrew Casey (amcasey) · 2017-08-21T23:55:43Z

+        const regions: RegionRange[] = [];
+        const regionText = "#region";
+        const regionStart = new RegExp("^\\s*//\\s*#region(\\s+.*)?$", "gm");
+        const regionEnd = new RegExp("^\\s*//\\s*#endregion(\\s|$)", "gm");


\s [](start = 60, length = 3)

Exactly one? #Closed

Yes; if the line ends after #endregion, it's fine, and if there's whitespace and then anything else, it's still fine, since we just want to know that the key phrase is actually there. #Closed

Ah, I missed that this was a substring match (i.e. that the end of the line was open-ended).

Andrew Casey (amcasey) · 2017-08-21T23:56:53Z

        let depth = 0;
+        const regions: RegionRange[] = [];
+        const regionText = "#region";
+        const regionStart = new RegExp("^\\s*//\\s*#region(\\s+.*)?$", "gm");


gm [](start = 72, length = 2)

What does this do?

Removed the flags :) they weren't necessary anymore.

Andrew Casey (amcasey) · 2017-08-21T23:58:00Z

+        const regions: RegionRange[] = [];
+        const regionText = "#region";
+        const regionStart = new RegExp("^\\s*//\\s*#region(\\s+.*)?$", "gm");
+        const regionEnd = new RegExp("^\\s*//\\s*#endregion(\\s|$)", "gm");


endregion [](start = 50, length = 9)

Is there any way to associate the region's identifier with the #endregion? #Closed

Yes, you can put anything you want there.

Andrew Casey (amcasey) · 2017-08-21T23:58:22Z

        let depth = 0;
+        const regions: RegionRange[] = [];
+        const regionText = "#region";
+        const regionStart = new RegExp("^\\s*//\\s*#region(\\s+.*)?$", "gm");


region [](start = 52, length = 6)

Case sensitive? #Closed

C# is case sensitive, so we're matching them. #Closed

Andrew Casey (amcasey) · 2017-08-21T23:59:07Z

+        function addOutliningSpanRegions(regionSpan: RegionRange) {
+            if (regionSpan) {
+                const span: OutliningSpan = {
+                    textSpan: createTextSpanFromBounds(regionSpan.pos, regionSpan.end),


createTextSpanFromBounds(regionSpan.pos, regionSpan.end) [](start = 30, length = 56)

Extract common sub-expression?

Andrew Casey (amcasey) · 2017-08-22T00:00:55Z

            return isFunctionBlock(node) && node.parent.kind !== SyntaxKind.ArrowFunction;
        }

+        function isRegionStart(start: number, end: number) {


isRegionStart [](start = 17, length = 13)

It would appear to be a precondition that start is within a comment. Consider adding a comment to that effect.

See new approach.

Andrew Casey (amcasey) · 2017-08-22T00:01:46Z

+                const result = comment.match(regionStart);
+
+                if (result && result.length > 0) {
+                    const sections = result[0].split(" ").filter(function (s) { return s !== ""; });


" " [](start = 53, length = 3)

Why space?

Seems like you want to split on any whitespace.

See new capture group approach.

Andrew Casey (amcasey) · 2017-08-22T00:02:06Z

+                const result = comment.match(regionStart);
+
+                if (result && result.length > 0) {
+                    const sections = result[0].split(" ").filter(function (s) { return s !== ""; });


function [](start = 65, length = 8)

Arrow expressions are nice in cases like this.

Andrew Casey (amcasey) · 2017-08-22T00:03:50Z

+            const lineStarts = sourceFile.getLineStarts();
+
+            for (const currentLineStart of lineStarts) {
+                const lineEnd = sourceFile.getLineEndOfPosition(currentLineStart);


getLineEndOfPosition [](start = 43, length = 20)

Couldn't you use the start position of the next line?

Using start position of next line now.

Andrew Casey (amcasey) · 2017-08-22T00:04:31Z

            return isFunctionBlock(node) && node.parent.kind !== SyntaxKind.ArrowFunction;
        }

+        function isRegionStart(start: number, end: number) {


isRegionStart [](start = 17, length = 13)

This is a confusing name for a method that returns the name of the region.

Andrew Casey (amcasey)

🕐

Andrew Casey (amcasey)

LGTM, with minor comments

Bill Ticehurst (billti) · 2017-08-22T17:28:35Z

+        const regions: RegionRange[] = [];
+        const regionText = "#region";
+        const regionStart = new RegExp("^\\s*//\\s*#region(\\s+.*)?$", "gm");
+        const regionEnd = new RegExp("^\\s*//\\s*#endregion(\\s|$)", "gm");


Seeing as you now search line-by-line, as opposed to over the whole file, there is no value in the 'g' and 'm' flags on the regular expression. You are only looking for one match on one line at a time.

Arthur Ozga (aozgaa) · 2017-08-22T19:24:29Z

+////
+////// #endregion outer|]
+////
+////// region delimiters not valid when preceding text on line


"when there is"?

Arthur Ozga (aozgaa) · 2017-08-22T19:26:21Z

@@ -0,0 +1,46 @@
+/// <reference path="fourslash.ts"/>
+
+////// basic region


un-named/anonymous region?

Renamed "region without label"

Arthur Ozga (aozgaa) · 2017-08-22T19:28:19Z

+        function addOutliningSpanRegions(regionSpan: RegionRange) {
+            if (regionSpan) {
+                const span: OutliningSpan = {
+                    textSpan: createTextSpanFromBounds(regionSpan.pos, regionSpan.end),


Use createTextSpanFromRange?

Arthur Ozga (aozgaa) · 2017-08-22T19:32:29Z

        }

+        function isRegionStart(start: number, end: number) {
+            if (!ts.formatting.getRangeOfEnclosingComment(sourceFile, start, /*onlyMultiLine*/ true)) {


There is also an isInComment helper which coerces the result into a boolean, but then you would effectively be doing a triple-negation. I'm still inclined to switching to the helper, but this is mostly a matter of taste.

Used isInComment.

Arthur Ozga (aozgaa) · 2017-08-22T19:43:27Z

        const elements: OutliningSpan[] = [];
        let depth = 0;
+        const regions: RegionRange[] = [];
+        const regionText = "#region";


Maybe rename to regionDelimiter or regionStartDelimter? It's a bit confusing when reading isRegionStart below.

Changed to "defaultLabel"

Arthur Ozga (aozgaa) · 2017-08-22T19:48:24Z

+            return "";
+        }
+
+        function isRegionEnd(start: number, end: number) {


does this return a boolean? Please rename if not, or coerce the result to a boolean via !!

Mohamed Hegazy (mhegazy) · 2017-09-15T16:40:29Z

        end: -1;
    }

+    export interface RegionRange extends TextRange {


should this be used in the definition of OutliningSpan now?

also please move this to services/types.ts instead.

i see now this is only used internally while building the spans. then this should move to src/services/outliningElementsCollector.ts and not be exported.

Mohamed Hegazy (mhegazy) · 2017-09-15T17:46:16Z

+                const currentLineStart = lineStarts[i];
+                const lineEnd = lineStarts[i + 1] - 1 || sourceFile.getEnd();
+
+                const name = getRegionName(currentLineStart, lineEnd);


looks like you are calling isInComment a few times every line. I would say this is more expensive than the other checks, which is basically does the line start with // or not.

consider making this check first. then after that run your regexp, and if that all passes, then call isInComment before adding the region.

Also consider combining your regexps into one. this way you run the regexp and gets you name + end/start marker.

In other words, combine isRegionEnd and getRegionName and inline them in this loop.

Combined the regexp and moved the isInComment after we know it's a relevant line.

Mohamed Hegazy (mhegazy) · 2017-09-15T18:05:55Z

        }

+        function addOutliningSpanRegions(regionSpan: RegionRange) {
+            if (regionSpan) {


why not just create OutliningSpans for every region when we are building them instead of creating the extra object. less garbage to collect later.

Mohamed Hegazy (mhegazy) · 2017-09-15T23:30:54Z

    const collapseText = "...";
    const maxDepth = 20;
+    const defaultLabel = "#region";
+    const regionMatch = new RegExp("^\\s*//\\s*(#region|#endregion)(?:\\s+(.*))?$");


consider making end optional, this way you can just check on the truthiness of result[1]
new RegExp("^\\s*//\\s*#(end)?region(?:\\s+(.*))?$");

Mohamed Hegazy (mhegazy) · 2017-09-15T23:32:34Z

+                    else {
+                        const region = regions.pop();
+                        if (region) {
+                            const newTextSpan = createTextSpanFromBounds(region.textSpan.start, lineEnd);


why not just region.textSpan.length = linEnd -region.textSpan.start; and avoid creating the new object.

Holger Jeromin (HolgerJeromin) · 2017-10-13T08:25:59Z

Is this a fix for VS Code, VS 2015 and/or VS2017?

Ben Lichtman (uniqueiniquity) · 2017-10-13T15:37:31Z

This is a fix for VS2017 and other editors that gather collapsing regions using the TS language service.
I believe VS Code just released their own version: https://code.visualstudio.com/updates/v1_17#_folding-regions.

Ben Lichtman (uniqueiniquity) added 8 commits August 21, 2017 15:43

Add support for custom outlining regions

9726ba1

Add regex sweep implementation

f91c23b

Fix name capture logic

0b3ec24

Clean up unused functions

0ef5498

Ensure region boundaries are entire line

4971e31

Exclude region delimiters in multiline comments

562d988

Ensure returned spans are ordered by start

fb462c9

Add test for region spans

442bc56

Ben Lichtman (uniqueiniquity) requested review from Andrew Casey (amcasey), Arthur Ozga (aozgaa), Bill Ticehurst (billti) and Mohamed Hegazy (mhegazy) August 21, 2017 22:54

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Aug 21, 2017

Bill Ticehurst (billti) reviewed Aug 21, 2017

View reviewed changes

Bill Ticehurst (billti) approved these changes Aug 21, 2017

View reviewed changes

Andrew Casey (amcasey) reviewed Aug 21, 2017

View reviewed changes

Andrew Casey (amcasey) reviewed Aug 22, 2017

View reviewed changes

Andrew Casey (amcasey) suggested changes Aug 22, 2017

View reviewed changes

Andrew Casey (amcasey) approved these changes Aug 22, 2017

View reviewed changes

Bill Ticehurst (billti) reviewed Aug 22, 2017

View reviewed changes

Arthur Ozga (aozgaa) reviewed Aug 22, 2017

View reviewed changes

Region now starts at beginning of comment, and reviewer edits

509d347

Bill Ticehurst (billti) approved these changes Aug 22, 2017

View reviewed changes

Edits from aozgaa review and simplify regex

c3f2648

Mohamed Hegazy (mhegazy) reviewed Sep 15, 2017

View reviewed changes

Move RegionRange to private scope

7781245

Mohamed Hegazy (mhegazy) reviewed Sep 15, 2017

View reviewed changes

Combine and simplify regex

3dfeb2d

Ben Lichtman (uniqueiniquity) force-pushed the regions branch from 9c25611 to 3dfeb2d Compare September 15, 2017 22:52

Refactored out RegionRange

484bd20

Mohamed Hegazy (mhegazy) reviewed Sep 15, 2017

View reviewed changes

Mohamed Hegazy (mhegazy) approved these changes Sep 15, 2017

View reviewed changes

Remove extra OutliningSpan and simplify regex

e5c43cd

Ben Lichtman (uniqueiniquity) merged commit 83f2401 into microsoft:master Sep 16, 2017

Microsoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018

		@@ -0,0 +1,46 @@
		/// <reference path="fourslash.ts"/>

		////// basic region

Conversation

Ben Lichtman (uniqueiniquity) commented Aug 21, 2017

Uh oh!

Microsoft Contribution License Agreements (msftclas) commented Aug 21, 2017 • edited by amcasey Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bill Ticehurst (billti) left a comment

Choose a reason for hiding this comment

Uh oh!

Andrew Casey (amcasey) Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ben Lichtman (uniqueiniquity) Aug 22, 2017 • edited by amcasey Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew Casey (amcasey) Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew Casey (amcasey) Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ben Lichtman (uniqueiniquity) Aug 22, 2017 • edited by amcasey Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew Casey (amcasey) left a comment

Choose a reason for hiding this comment

Uh oh!

Microsoft Contribution License Agreements (msftclas) commented Aug 21, 2017 •

edited by amcasey

Loading

Andrew Casey (amcasey) Aug 21, 2017 •

edited

Loading

Ben Lichtman (uniqueiniquity) Aug 22, 2017 •

edited by amcasey

Loading

Andrew Casey (amcasey) Aug 21, 2017 •

edited

Loading

Andrew Casey (amcasey) Aug 21, 2017 •

edited

Loading

Ben Lichtman (uniqueiniquity) Aug 22, 2017 •

edited by amcasey

Loading