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.

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

Add support for custom outlining regions #17954

Merged
merged 14 commits into from
Sep 16, 2017
Merged

Add support for custom outlining regions #17954

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9726ba1
Add support for custom outlining regions
uniqueiniquity Aug 8, 2017
f91c23b
Add regex sweep implementation
uniqueiniquity Aug 8, 2017
0b3ec24
Fix name capture logic
uniqueiniquity Aug 9, 2017
0ef5498
Clean up unused functions
uniqueiniquity Aug 9, 2017
4971e31
Ensure region boundaries are entire line
uniqueiniquity Aug 9, 2017
562d988
Exclude region delimiters in multiline comments
uniqueiniquity Aug 21, 2017
fb462c9
Ensure returned spans are ordered by start
uniqueiniquity Aug 21, 2017
442bc56
Add test for region spans
uniqueiniquity Aug 21, 2017
509d347
Region now starts at beginning of comment, and reviewer edits
uniqueiniquity Aug 22, 2017
c3f2648
Edits from aozgaa review and simplify regex
uniqueiniquity Aug 22, 2017
7781245
Move RegionRange to private scope
uniqueiniquity Sep 15, 2017
3dfeb2d
Combine and simplify regex
uniqueiniquity Sep 15, 2017
484bd20
Refactored out RegionRange
uniqueiniquity Sep 15, 2017
e5c43cd
Remove extra OutliningSpan and simplify regex
uniqueiniquity Sep 15, 2017
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
40 changes: 39 additions & 1 deletion src/services/outliningElementsCollector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@
namespace ts.OutliningElementsCollector {
const collapseText = "...";
const maxDepth = 20;
const defaultLabel = "#region";
const regionMatch = new RegExp("^\\s*//\\s*(#region|#endregion)(?:\\s+(.*))?$");
Copy link
Contributor

Choose a reason for hiding this comment

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

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


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

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 Down Expand Up @@ -89,6 +93,40 @@ namespace ts.OutliningElementsCollector {
return isFunctionBlock(node) && node.parent.kind !== SyntaxKind.ArrowFunction;
}

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 comment = sourceFile.text.substring(currentLineStart, lineEnd);
const result = comment.match(regionMatch);

if (result && !isInComment(sourceFile, currentLineStart)) {
if (result[1] === "#region") {
const start = sourceFile.getFullText().indexOf("//", currentLineStart);
const textSpan = createTextSpanFromBounds(start, lineEnd);
const region: OutliningSpan = {
textSpan,
hintSpan: textSpan,
bannerText: result[2] || defaultLabel,
autoCollapse: false
};
regions.push(region);
}
else {
const region = regions.pop();
if (region) {
const newTextSpan = createTextSpanFromBounds(region.textSpan.start, lineEnd);
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 region.textSpan.length = linEnd -region.textSpan.start; and avoid creating the new object.

region.textSpan = newTextSpan;
region.hintSpan = newTextSpan;
elements.push(region);
}
}
}
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. :)

}
}

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

////// region without label
////[|// #region
////
////// #endregion|]
////
////// region without label with trailing spaces
////[|// #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
View file
Open in desktop
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
View file
Open in desktop
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());