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 8 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
4 changes: 4 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2037,6 +2037,10 @@ namespace ts {
end: -1;
}

export interface RegionRange extends TextRange {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be used in the definition of OutliningSpan now?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved.

name?: string;
}

// represents a top level: { type } expression in a JSDoc comment.
export interface JSDocTypeExpression extends TypeNode {
kind: SyntaxKind.JSDocTypeExpression;
Expand Down
82 changes: 81 additions & 1 deletion src/services/outliningElementsCollector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,14 @@ namespace ts.OutliningElementsCollector {
export function collectElements(sourceFile: SourceFile, cancellationToken: CancellationToken): OutliningSpan[] {
const elements: OutliningSpan[] = [];
let depth = 0;
const regions: RegionRange[] = [];
const regionText = "#region";
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 "defaultLabel"

const regionStart = new RegExp("^\\s*//\\s*#region(\\s+.*)?$", "gm");
Copy link
Member

Choose a reason for hiding this comment

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

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@amcasey amcasey Aug 21, 2017

Choose a reason for hiding this comment

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

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

Case sensitive? #Closed

Copy link
Contributor Author

@uniqueiniquity uniqueiniquity Aug 22, 2017

Choose a reason for hiding this comment

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

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

const regionEnd = new RegExp("^\\s*//\\s*#endregion(\\s|$)", "gm");
Copy link
Member

@amcasey amcasey Aug 21, 2017

Choose a reason for hiding this comment

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

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

Exactly one? #Closed

Copy link
Contributor Author

@uniqueiniquity uniqueiniquity Aug 22, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@amcasey amcasey Aug 21, 2017

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you can put anything you want there.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.


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 +40,18 @@ 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 span: OutliningSpan = {
textSpan: createTextSpanFromBounds(regionSpan.pos, regionSpan.end),
Copy link
Member

Choose a reason for hiding this comment

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

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

Extract common sub-expression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use createTextSpanFromRange?

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.

hintSpan: createTextSpanFromBounds(regionSpan.pos, regionSpan.end),
bannerText: regionSpan.name,
autoCollapse: false,
};
elements.push(span);
}
}

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

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

function isRegionStart(start: number, end: number) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See new approach.

Copy link
Member

Choose a reason for hiding this comment

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

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

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

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.

if (!ts.formatting.getRangeOfEnclosingComment(sourceFile, start, /*onlyMultiLine*/ true)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used isInComment.

const comment = sourceFile.text.substring(start, end);
const result = comment.match(regionStart);

if (result && result.length > 0) {
const sections = result[0].split(" ").filter(function (s) { return s !== ""; });
Copy link
Member

Choose a reason for hiding this comment

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

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

Why space?

Copy link
Member

Choose a reason for hiding this comment

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

Seems like you want to split on any whitespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See new capture group approach.

Copy link
Member

Choose a reason for hiding this comment

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

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

Arrow expressions are nice in cases like this.


if (sections[0] === "//") {
if (sections.length > 2) {
return result[0].substring(result[0].indexOf(sections[2]));
}
else {
return regionText;
}
}
else {
if (sections.length > 1) {
return result[0].substring(result[0].indexOf(sections[1]));
}
else {
return regionText;
}
}
}
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 (!ts.formatting.getRangeOfEnclosingComment(sourceFile, start, /*onlyMultiLine*/ true)) {
const comment = sourceFile.text.substring(start, end);
return comment.match(regionEnd);
}
return undefined;
}

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

for (const currentLineStart of lineStarts) {
const lineEnd = sourceFile.getLineEndOfPosition(currentLineStart);
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using start position of next line now.


const name = isRegionStart(currentLineStart, lineEnd);
if (name) {
const region: RegionRange = {
pos: currentLineStart,
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"/>

////// basic region
Copy link
Contributor

Choose a reason for hiding this comment

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

un-named/anonymous region?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed "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 preceding text on line
Copy link
Contributor

Choose a reason for hiding this comment

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

"when there is"?

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.

//// 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());