Skip to content
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.

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

Add support for custom outlining regions #17954

Merged
merged 14 commits into from
Sep 16, 2017
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
78 changes: 77 additions & 1 deletion src/services/outliningElementsCollector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,22 @@
namespace ts.OutliningElementsCollector {
const collapseText = "...";
const maxDepth = 20;
const defaultLabel = "#region";
const regionStart = new RegExp("^//\\s*#region(\\s+.*)?$");
const regionEnd = new RegExp("^//\\s*#endregion(\\s|$)");

interface RegionRange extends TextRange {
name?: string;
}

export function collectElements(sourceFile: SourceFile, cancellationToken: CancellationToken): OutliningSpan[] {
const elements: OutliningSpan[] = [];
let depth = 0;
const regions: RegionRange[] = [];

walk(sourceFile);
return elements;
gatherRegions();
return elements.sort((span1, span2) => span1.textSpan.start - span2.textSpan.start);

/** If useFullStart is true, then the collapsing span includes leading whitespace, including linebreaks. */
function addOutliningSpan(hintSpanNode: Node, startElement: Node, endElement: Node, autoCollapse: boolean, useFullStart: boolean) {
Expand All @@ -35,6 +44,19 @@ namespace ts.OutliningElementsCollector {
}
}

function addOutliningSpanRegions(regionSpan: RegionRange) {
if (regionSpan) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

const textSpan = createTextSpanFromRange(regionSpan);
const span: OutliningSpan = {
textSpan,
hintSpan: textSpan,
bannerText: regionSpan.name,
autoCollapse: false,
};
elements.push(span);
}
}

function addOutliningForLeadingCommentsForNode(n: Node) {
const comments = ts.getLeadingCommentRangesOfNode(n, sourceFile);

Expand Down Expand Up @@ -89,6 +111,60 @@ namespace ts.OutliningElementsCollector {
return isFunctionBlock(node) && node.parent.kind !== SyntaxKind.ArrowFunction;
}

function getRegionName(start: number, end: number) {
if (!isInComment(sourceFile, start)) {
const comment = sourceFile.text.substring(start, end).trim();
const result = comment.match(regionStart);

if (result && result.length > 0) {
const label = result.pop();
if (label) {
return label.trim();
}
else {
return defaultLabel;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to use capture group.

Copy link
Member

Choose a reason for hiding this comment

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

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I've actually pushed the change. :)

}
return "";
}

function isRegionEnd(start: number, end: number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coerced.

if (!isInComment(sourceFile, start)) {
const comment = sourceFile.text.substring(start, end).trim();
return !!comment.match(regionEnd);
}
return false;
}

function gatherRegions(): void {
const lineStarts = sourceFile.getLineStarts();

for (let i = 0; i < lineStarts.length; i++) {
const currentLineStart = lineStarts[i];
const lineEnd = lineStarts[i + 1] - 1 || sourceFile.getEnd();

const name = getRegionName(currentLineStart, lineEnd);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@mhegazy mhegazy Sep 15, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if (name) {
const start = sourceFile.getFullText().indexOf("//", currentLineStart);
const region: RegionRange = {
pos: start,
end: lineEnd,
name,
};
regions.push(region);
}
else if (isRegionEnd(currentLineStart, lineEnd)) {
const region = regions.pop();

if (region) {
region.end = lineEnd;
addOutliningSpanRegions(region);
}
}
}
}

function walk(n: Node): void {
cancellationToken.throwIfCancellationRequested();
if (depth > maxDepth) {
Expand Down
46 changes: 46 additions & 0 deletions tests/cases/fourslash/getOutliningSpansForRegions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/// <reference path="fourslash.ts"/>

////// region without label
////[|// #region
////
////// #endregion|]
////
////// region with label
////[|// #region label1
////
////// #endregion|]
////
////// region with extra whitespace in all valid locations
//// [|// #region label2 label3
////
//// // #endregion|]
////
////// No space before directive
////[|//#region label4
////
//////#endregion|]
////
////// Nested regions
////[|// #region outer
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

////
////[|// #region inner
////
////// #endregion inner|]
////
////// #endregion outer|]
////
////// region delimiters not valid when there is preceding text on line
//// test // #region invalid1
////
////test // #endregion
////
////// region delimiters not valid when in multiline comment
/////*
////// #region invalid2
////*/
////
/////*
////// #endregion
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added unbalanced tests.

////*/

verify.outliningSpansInCurrentFile(test.ranges());
10 changes: 10 additions & 0 deletions tests/cases/fourslash/getOutliningSpansForUnbalancedEndRegion.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/// <reference path="fourslash.ts"/>

////// bottom-heavy region balance
////[|// #region matched
////
////// #endregion matched|]
////
////// #endregion unmatched

verify.outliningSpansInCurrentFile(test.ranges());
11 changes: 11 additions & 0 deletions tests/cases/fourslash/getOutliningSpansForUnbalancedRegion.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/// <reference path="fourslash.ts"/>

////// top-heavy region balance
////// #region unmatched
////
////[|// #region matched
////
////// #endregion matched|]

debugger;
verify.outliningSpansInCurrentFile(test.ranges());