(GH-1336) Add syntax aware folding provider #1355

glennsarti · 2018-06-09T13:03:46Z

PR Summary

Adds a syntax aware folding provider, hidden behind a feature flag.

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.

Outstanding items

Outstanding style questions about .forEach
Question about moving getCoreNodeModule into util.js
Question about whether to convert FP style to imperative for the powerShellGrammarPath function

Required features

Functions (Fixed in { } support)
Regions (#region and # region)
Here strings
Comment Blocks (# .... \n# ....>)
Comment based help (<# .... #>)
Make folding provider optional (via config)

Additional features added

Hashtable folding (From { } support)
Array folding (From ( ) support)

PR Review requirements

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
MEGA BONUS POINTS - It has tests!!
Add logging in the provider/feature
Converting comments into JSDoc format
Convert to Literal regex

glennsarti · 2018-06-09T13:07:28Z

This is purely just to show the work I've done so far. It is far from complete (and breaks about 10,000 typescript linting rules :-) )

My current thinking is;

Use the client side textmate grammar file to tokenize the source
Once I have that, I can make decisions as to where code begins
Once I know where code begins and ends I can craft the required FoldingRange objects

Note that the syntax folding supersedes any "default" folding regions e.g. # region or line break/indentation style.

glennsarti · 2018-06-10T13:55:13Z

I have brackets ( { } and ( ) ) working

It's hard to see but...you'll notice the default folding is there until the extension is loaded, which "magically" removes the folding for the # region section.

rkeithhill · 2018-06-10T17:59:23Z

Use the client side textmate grammar file to tokenize the source
Once I have that, I can make decisions as to where code begins

It sounds like you are redoing work the parser has already done. For example, getting here string folding region info amounts to this:

$tok = $err = $null
$ast = [System.Management.Automation.Language.Parser]::ParseFile("$pwd\examples\DebugTest.ps1", [ref]$tok, [ref]$err)
$ast.FindAll({param($ast) ($ast -is [System.Management.Automation.Language.StringConstantExpressionAst]) -and (($ast.StringConstantType -eq [System.Management.Automation.Language.StringConstantType]::DoubleQuotedHereString) -or ($ast.StringConstantType -eq [System.Management.Automation.Language.StringConstantType]::SingleQuotedHereString))}, $true) | 
    Foreach-Object Extent |
    Select-Object StartLineNumber, EndLineNumber

Which, for my test file outputs:

StartLineNumber EndLineNumber
--------------- -------------
              3             9
             34            36

We have the ability from the TS extension source to make a request over to the PS Editor Services such that it can execute PowerShell code for us including queries on the AST. This is how the code analysis diagnostics are reported. This is how help completion works. This is how "Find PS modules from Gallery" works. The one area that the AST doesn't help with is comments which includes region support - since that is implemented as "special" comment. Anyway, I had envisioned the folding support would take advantage of the AST for most everything else.

If you need help with the infrastructure to callback to PSES as well as the PSES C# infrastructure to process the request - let me know.

glennsarti · 2018-06-11T01:55:05Z

Hi @rkeithhill . Thanks for taking the time to look at my WIP. I did consider using the Language Server to surface this information but I decided against it for the following reasons:

If the language server crashes, you lose folding. This is a "breaking change" behaviour of the extension, where folding still works (Indentation based) if the lang. server is unavailable
VS Code is the only editor I can see that allows syntax aware folding (Atom and Sublime do not. Didn't check VIM). So this feature is specific to VSCode so it seems to belong in the vscode extension, not the generic Editor Services.
You can get the region comments by using the tokens generated by the ::ParseFile method ($tok). But at this point you're replicating things in the lang. server you could do client side.
Reduce complexity. By adding another "thing" to the lang. server. including all the work to add that "thing", and all the plumbing that goes with it, is the increased complexity (and to some degree latency) worth it?. In my opinion, no. We can do the same work client side (using the grammar file), with almost exact accuracy as the native PowerShell parser, without the complexity overhead.

Note - By complexity, I'm more referring to the system as a whole, as opposed to complex for a developer to write/understand.

(This point is debatable) I feel it makes more sense for the token decisions to come from the textmate grammar than the lang. server.

Folding should follow the visual cues of the editor. The visual cues come from the colourisation and syntax highlighting, which come the textmate grammar. These cues do not come from the lang. server. But ... you could easily argue that the lang. server knows more about the file and should be considered the authority.

Of course there are downsides to going client side:

The powershell grammar tokens I'm looking for would become hard-coded. So if there were changes to the scope names in the grammar file, the folding provider would fail to see them. This means the client side method is fragile to grammar scope name changes.

Now, is this possible? of course it's possible we'll have changes. Is this probable? For the tokens I'm looking for, not likely. Is this testable? Yes, tests could be written to make sure the folding provider continues to function as expected.

rkeithhill · 2018-06-11T02:39:05Z

If the language server crashes, you lose folding.

And intellisense, code analysis, code formatting, code lens, etc. When the language server crashes, it's a bad day. I don't think that's a good reason to not utilize the language server for some of this.

you could easily argue that the lang. server knows more about the file and should be considered the authority.

Yup. In addition, the tm grammar we've been using has been a steady source of issues/complaints. I'm not particularly confident in its ability to support PowerShell adequately. This bug was fixed recently (the = after ParameterSetName throws it off):

For the longest time, Where-Object, Sort-Object, Select-Object wouldn't get colorized correctly. That was recently fixed but we still get a pretty steady stream of tm grammar issues - see https://github.com/powershell/editorSyntax/issues

I'm probably more than a little jaded about the tm grammar. :-) Heck, if I had my druthers we'd use an API like what is being proposed here: microsoft/vscode#585 Which is essentially how Visual Studio does syntax colorization. It asks the language service to provide colorization info.

By adding another "thing" to the lang. server. including all the work to add that "thing", and all the plumbing that goes with it, is the increased complexity (and to some degree latency) worth it?

Depends. If you can provide all the desired folding regions without all the corner cases bugs we seem to so frequently run into with the tm grammar, then no, it's not worth using the AST via the lang server. OTOH I'd have more confidence in the information we get from AST i.e. I wouldn't worry about corner-cases. But I'm willing to be proven wrong. It could be my low opinion of the tm grammar is unwarranted. :-)

BTW, lest I come off too negative, I really do appreciate the contribution!! Proper here string folding is something folks have been asking for, for a long time now. Just want to make sure we have a healthy debate about the best approach and wanted to make you're aware that using the AST is a fairly easy option.

glennsarti · 2018-06-11T04:05:11Z

Just want to make sure we have a healthy debate about the best approach

Agreed! FWIW I'm one of the authors of the Puppet extension and I have the same debates where to stick features client vs Lang. Server. Always trade-offs

There is also a middle ground, which is to implement client side until the server side code is complete, and then just switch over. Could even hide it behind a vscode setting to make it an opt-in feature. Adding a command to the language server is not trivial, to me anyway. I am not usually a C# developer so the ramp-up time is not small. The plumbing to call the server from the client I can handle ok (copy-n-paste to a degree). If it's decided to go down that route, not client side, I probably won't be able to assist.

Regarding the TM issues... I personally haven't had them, however I too am just a sample size of one :-)

If you can provide all the desired folding regions without all the corner cases bugs we seem to so frequently run into

For the tokens we're interested in in this PR, I don't feel it's too bad, but that's just a gut feel, no data to back it up.

So to wrap up.

The Language Server is probably the final resting place for the folding logic to be determined, and right now I will not be able to assist with that.
But I, as a community member, can provide an interim solution using the grammar file for tokenisation, until the more correct solution (lang server) is implemented. The interim solution can be opt-in which should help with supporting the interim feature.
Switching over from the client side to server side logic will be transparent to the vscode users.

rkeithhill · 2018-06-11T04:23:54Z

Sounds good. If I can carve off some time to work on the PSES impl, what info does the client need besides start/end line pairs?

glennsarti · 2018-06-11T04:26:52Z

Needs enough information to populate - https://code.visualstudio.com/docs/extensionAPI/vscode-api#FoldingRange

Start
End
Type of region - Comment, Imports (N/A to us) and Region.

rkeithhill · 2018-06-11T04:31:36Z

Interesting. So what FoldingRangeKind do you use for functions and here strings?

glennsarti · 2018-06-11T04:32:40Z

I was going to use 'Region' for everything except comments

rkeithhill · 2018-06-11T04:38:03Z

I wonder if we should provide feedback to the VSCode team that we'd like to have a few more "kinds" e.g. function/method, verbatim/here string, or maybe just "Other" as a catchall.

glennsarti · 2018-06-11T04:54:50Z

Sure. I hadn't thought that far ahead.

TylerLeonhardt · 2018-06-11T06:43:27Z

Awesome discussion guys :) I agree that PSES should be, in the end, where the folds come from. Another benefit I don't think was mentioned is that other editors could implement syntax aware folding and those editors can get it for free/minimal effort :)

It would also be good to have all the using statements in PowerShell folded as an imports type but this is much lower pri IMO.

rjmholt · 2018-06-11T21:28:04Z

Hmmmm I definitely see the motivation to make folding all happen client side. Especially since folding should be simple.

But the truth is that trying to do it that way we will inevitably come across things that a TM Grammar is simply not powerful enough to recognise - PowerShell is woefully grammatically complex. The parser is the final arbiter of what text is what token, and if we handle ASTs properly (i.e. cache them, etc), it should all be very cheap too.

@rkeithhill as an aside: I too really want that hook-in highlighting functionality - would be very useful for things like dynamic keyword support and function highlighting.

glennsarti · 2018-06-13T14:38:54Z

After happens when the following is in the user settings

    "powershell.developer.featureFlags": [
        "syntax-folding"
    ],

Before	After

glennsarti · 2018-06-13T14:40:03Z

@rkeithhill and @rjmholt PR is done. Ready for review.

TylerLeonhardt · 2018-06-13T16:09:37Z

@omniomi is the big contributor to EditorSyntax which is where the syntax highlighting comes from.

Nick, if you have some time, can you take a look at this PR and give your thoughts on this since this change depends on EditorSyntax?

omniomi · 2018-06-13T17:03:17Z

From a quick look over none of the scopes used for folding in this PR are likely to change as they were all corrected in an older PR. That said, @glennsarti I'll add begin and end scopes to here-strings which I just noticed are missing.

@tylerl0706 I think we're at the point we should create a dependency document for EditorSyntax to track the potential fallout from changes. Ie, where it's used, any specific scope dependencies, as well as processes for having dependents update to newer revisions.

TylerLeonhardt

I've got a few comments but it all looks really good!

TylerLeonhardt · 2018-06-13T16:19:47Z

src/features/Folding.ts

+                document: vscode.TextDocument) {
+        this.start = start;
+        this.end = end;
+        if (startLine === null) {


You could use a ternary operator here:

this.startline = startLine ? startLine : document.positionAt(start.startIndex).line;

I was going to suggest ||, but both suggestions are wrong since startLine can be 0.

Needs to be:

this.startLine = startLine !== null ? startLine : document.positionAt(start.startIndex).line;

And if there's a worry about the possibility of undefined being a possible value of startLine, == will work for that.

No longer an issue.

TylerLeonhardt · 2018-06-13T16:19:59Z

src/features/Folding.ts

+        } else {
+            this.startline = startLine;
+        }
+        if (endLine === null) {


same ternary op.

No longer an issue.

TylerLeonhardt · 2018-06-13T17:15:59Z

src/features/Folding.ts

+        document: vscode.TextDocument,
+        context: vscode.FoldingContext,
+        token: vscode.CancellationToken,
+    ): Promise<vscode.FoldingRange[]> {


nitpick: I noticed some of your functions look like this:

public foo( doc: bar context: baz ) : Boo { // ... }

and some look like:

public foo(doc: bar context: baz) : Boo { // ... }

Is there a reason for the different indenting? Can we align those if not? (I personally prefer the first style 😄)

It's my ruby-isms coming through :-). Will reformat to first one :-)

Somewhat fixed. I've reformatted the code I wrote, haven't done the code I copy-pasta'd from other sources.

TylerLeonhardt · 2018-06-13T17:22:11Z

src/features/Folding.ts

+            }
+        });
+
+        return result.reverse();


if you need to return the reversed result, maybe use result.unshift instead of result.push?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/unshift

TylerLeonhardt · 2018-06-13T17:30:19Z

src/features/Folding.ts

+    }
+
+    // Given a zero based offset, return the line number in the document
+    private lineAtOffest(offset: number,


looks like a typo: Offest -> Offset

TylerLeonhardt · 2018-06-13T17:32:49Z

src/features/Folding.ts

+            }
+        });
+
+        // If we exist the token array and we're still processing comment lines, then the


did you mean "If we exit"?

TylerLeonhardt · 2018-06-13T17:35:48Z

src/features/Folding.ts

+                           document: vscode.TextDocument): string {
+        const startPos = document.positionAt(offset);
+        // We don't know how long the line is so just return a really long one.
+        const endPos = startPos.translate(0, 1000);


you can get the whole line with:

document.lineAt(document.positionAt(offset)).rangeIncludingLineBreak //or document.lineAt(document.positionAt(offset)).range // for no LineBreak

from that can we accurately get the start and end position?

rjmholt

This generally looks really great!

One thing we should consider is breaking up some of the denser sections of FP-style code into foreach loops, etc., just to align it more with the rest of the codebase and to make it easier to maintain for the maintainers.

Thanks so much for your contribution @glennsarti 😃!

rjmholt · 2018-06-13T17:58:12Z

package.json

@@ -5,7 +5,7 @@
  "publisher": "ms-vscode",
  "description": "Develop PowerShell scripts in Visual Studio Code!",
  "engines": {
-    "vscode": "^1.22.0"
+    "vscode": "^1.23.0"


Is the version increment required for this PR? I don't think we have a particular policy, but dependency version changes are important enough that it would be worth considering having that as its own PR.

Yes. The Folding API only exists in 1.23.0+

The previous changes didn't separate PRs for it.
790e6e2

5ad413c

#748

Ok, I'm convinced 😄

@tylerl0706 Maybe we should consider this a policy for the future, especially with all the feature work going into VSCode — I know .NET Core's latest update was surprisingly tricky for PowerShell.

I guess we could. We haven't gotten burned by it yet AFAIK but it doesn't hurt. Next time :)

Next time was last time :-P

rjmholt · 2018-06-13T17:59:55Z

src/features/Folding.ts

+const tm = getCoreNodeModule("vscode-textmate");
+// Returns a node module installed with VSCode, or null if it fails.
+// https://github.com/Microsoft/vscode/issues/46281
+function getCoreNodeModule(moduleName: string) {


A general function like this might fit better in utils.ts.

Probably, but this is a bit edge-case-y and is only consumed in this file. Would you still prefer I move it to utils?

@rjmholt I've moved that function as a private method within the feature (to help with loading). So it's no longer a general function. Given this, it's it still something I should change for this PR?

rjmholt · 2018-06-13T18:49:30Z

src/features/Folding.ts

+function getCoreNodeModule(moduleName: string) {
+    try {
+        return require(`${vscode.env.appRoot}/node_modules.asar/${moduleName}`);
+    // tslint:disable-next-line:no-empty


Rather than disabling tslint, we should probably log the inability to retrieve the module.

rjmholt · 2018-06-13T18:50:18Z

src/features/Folding.ts

+// Need to reproduce the IToken interface from vscode-textmate due to
+// the odd way it has to be required
+// https://github.com/Microsoft/vscode-textmate/blob/46af9487e1c8fa78aa1f2e2/release/main.d.ts#L161-L165
+export interface IToken {


I think ideally we should put exported definitions at the top of the file (in the same way as public members of an object).

Whoops, that interface is internal only. Removed the export.

rjmholt · 2018-06-13T18:58:07Z

src/features/Folding.ts

+}
+
+// The different types of matched textmate tokens.
+// Mirrors the enum at https://code.visualstudio.com/docs/extensionAPI/vscode-api#FoldingRangeKind


Ideally we should be using JSDoc-style comments for classes and methods. This project is a bit lacking in documentation and documenting comments, but this is what I imagine documentation looking like (not saying it's stellar - I wrote it - but the first example I could think of).

I think TypeScript has some integration with JSDoc comments that gives richer IDE integration as well. Here is a page that goes into more detail on JSDoc comments in TypeScript.

@tylerl0706 and @rkeithhill might have differing opinions on this btw 😄

I'd rather be consistently wrong, than wrong in inconsistent ways :-). I'll convert the comments over.

I'm open to using JSDoc comments on functions - where we actually document said functions. :-)

rjmholt · 2018-06-13T20:54:03Z

src/features/Folding.ts

+        try {
+            const psGrammars =
+                vscode.extensions.all
+                    .filter((x) => x.packageJSON && x.packageJSON.contributes && x.packageJSON.contributes.grammars)


The x, a and b in this method are probably a bit too terse, especially since they are not annotated with a type. Maybe give them expressive names and ideally type annotations.

It might also be worth taking the filter, reduce and map methods and turning them into more a verbose, imperative form, mainly because it's the dominant style elsewhere in the project. (Want to be clear that I'm philosophically in favour of a functional style, but want to unpack the functionality here a bit)

I changed the variable names. However I didn't convert to imperative, but instead, added code comments as breadcrumbs for future-us. Would that be enough?

rjmholt · 2018-06-13T21:03:42Z

src/features/Folding.ts

+            this.foldingProvider);
+    }
+
+    // tslint:disable-next-line:no-empty


It looks like these tslint pragmas might be necessary, but I would like to get rid of them if possible. I see there are recommendations to return undefined or void, but I'm not sold on that either. Thoughts?

~~returning void is probably fine.~~

Nope, tslint didn't like that. Just did return undefined; instead.

Fixed

rjmholt · 2018-06-13T21:06:03Z

src/features/Folding.ts

+
+}
+
+export class FoldingFeature implements IFeature {


This class should probably be defined at the top of the file.

Was copying the src/features/DocumentFormatter style.

rjmholt · 2018-06-13T21:09:59Z

src/features/Folding.ts

+                                "punctuation.section.group.begin.powershell",
+                                "punctuation.section.group.end.powershell",
+                                MatchType.Region, document)
+            .forEach((x) => { matchedTokens.push(x); });


The xs used here and below should be renamed more descriptively.

rjmholt · 2018-06-13T21:11:59Z

src/features/Folding.ts

+                document: vscode.TextDocument) {
+        this.start = start;
+        this.end = end;
+        if (startLine === null) {


I was going to suggest ||, but both suggestions are wrong since startLine can be 0.

Needs to be:

this.startLine = startLine !== null ? startLine : document.positionAt(start.startIndex).line;

And if there's a worry about the possibility of undefined being a possible value of startLine, == will work for that.

glennsarti · 2018-06-14T02:03:11Z

Yeah @omniomi , one thing that's missing from this PR is tests. In theory I should be able to write some simple integration tests (e.g. Given text content X, it should return Y folding regions).

For better or worse, as this is behind a feature flag, I don't need as good test coverage, yet!. This should give me enough time to figure out how to download a grammar test fixture, and write appropriate tests.

Previously the Powershell extension used the default VSCode indentation based folding regions. This commit adds the skeleton for a syntax aware, client-side folding provider as per the API introduced in VSCode 1.23.0 * The client side detection uses the PowerShell Textmate grammar file and the vscode-text node module to parse the text file into tokens which will be matched in later commits. * However due to the way vscode imports the vscode-textmate module we can't simply just import it, instead we need to use file based require statements microsoft/vscode#46281 * This also means it's difficult to use any typings exposed in that module. As we only need one interface, this is replicated verbatim in the file, but not exported * Logging is added to help diagnose potential issues

This commit adds detection of text regions bounded by braces { } and parentheses ( ). This provides syntax aware folding for functions, arrays and hash tables.

This commit adds detection of text regions composed of single and double quoted here strings; @' '@ and @" "@.

This commit adds syntax aware folding for comment regions * Contiguous blocks of line comments `# ....` * Block comments `<# ... #>` * Region bound comments `# region ... # endregion`

Previously the syntax folding was available for all users. However it was able to be configured or turned off. This commit adds a new `codeFolding` settings section, with the syntax folding feature enabled by default.

Previously there were no tests to verify the folding provider. Due to the provider depending on 3rd party libraries (vscode-textmate and PowerShell grammar file) these tests will provide a degree of detection if breaking changes occur.

Previously tslint was raising errors in Travis CI saying that the excluded test fixtures directory was not included in the project. This was by design however it appears to be a known bug palantir/tslint#3793. This commit removes the exclude for test files from linting and adds a tslint directive in the default index.ts file. A tslint directive is used instead of solving the issue because this is the default testing file for VS Code extesions and shouldn't really be modified unless absolutely necessary. In this instance it was safer for a tslint directive.

glennsarti · 2018-06-29T02:35:41Z

@tylerl0706 @rjmholt Addressed all comments. CI is green...

rkeithhill · 2018-06-29T02:42:11Z

src/features/Folding.ts

@@ -0,0 +1,588 @@
+import * as path from "path";


What's the story on source file copyright notices? Every other .ts file seems to have this copyright notice on the first line:

/*--------------------------------------------------------- * Copyright (C) Microsoft Corporation. All rights reserved. *--------------------------------------------------------*/

Even those contributed by community members.

❓ Contributing guide/s makes no mention. I've already signed the CLA.

I, as a non-Microsoft employee, shouldn't add that, as I don't represent them.

I don't mention this as a blocker but an inquiry as to what the policy is. Back in January I went through this code-base and cleaned up source to pass tslint as part of effort to achieve some form of engineering compliance (#1086). I'm curious if that "compliance" has anything to say about copyright notices.

I, as a non-Microsoft employee, shouldn't add that, as I don't represent them.

FWIW the copyright is their's either way, and ultimately the one representing Microsoft is whomever publishes the release.

I view it more as "informing of a copyright" rather than "declaring copyright".

That said, I'm not sure what the point of it is - at least in an MIT licensed project. Maybe @tylerl0706 or @rjmholt could check if there is a policy on this? If there isn't, my vote would be to remove it or at least omit it going forward.

(This is just a general note, definitely shouldn't hold up the PR)

added the headers

Btw, I looked at our contriibution guide and was about to add a note but wasn't sure of where to add it or how to phrase it. I also notice that PowerShell doesn't seem to have a note about it either, only in their PR checklist.

We could add it to the PR checklist :)

Offtopic - @SeeminglyScience - Ceeding copyright to Microsoft is fine. It was more the "All rights reserved". IANAL, but I didn't feel I was in position to represent to Microsoft to say what rights other people had. Again this all moot (Heh, I used legal term!) due to me signing the CLA.

@tylerl0706 @rjmholt Raised #1394 to add automation to enforce header rule.

rkeithhill

LGTM

rjmholt

LGTM! Thanks so much for your contribution @glennsarti!

TylerLeonhardt · 2018-07-01T21:43:37Z

Merging this in tomorrow morning :) unless people have further comments

glennsarti · 2018-07-02T00:27:50Z

TylerLeonhardt · 2018-07-02T18:35:06Z

It's "Squash" not "Smash" psh.... Hulk, please git good.

This is merged!! 😄

glennsarti · 2018-07-03T02:37:16Z

* (GH-1336) Add syntax aware folding provider Previously the Powershell extension used the default VSCode indentation based folding regions. This commit adds the skeleton for a syntax aware, client-side folding provider as per the API introduced in VSCode 1.23.0 * The client side detection uses the PowerShell Textmate grammar file and the vscode-text node module to parse the text file into tokens which will be matched in later commits. * However due to the way vscode imports the vscode-textmate module we can't simply just import it, instead we need to use file based require statements microsoft/vscode#46281 * This also means it's difficult to use any typings exposed in that module. As we only need one interface, this is replicated verbatim in the file, but not exported * Logging is added to help diagnose potential issues * (GH-1336) Add syntax folding for braces and parentheses This commit adds detection of text regions bounded by braces { } and parentheses ( ). This provides syntax aware folding for functions, arrays and hash tables. * (GH-1336) Add syntax folding for here strings This commit adds detection of text regions composed of single and double quoted here strings; @' '@ and @" "@. * (GH-1336) Add syntax folding for comments This commit adds syntax aware folding for comment regions * Contiguous blocks of line comments `# ....` * Block comments `<# ... #>` * Region bound comments `# region ... # endregion` * (GH-1336) Add integration tests for the Folding Provider Previously there were no tests to verify the folding provider. Due to the provider depending on 3rd party libraries (vscode-textmate and PowerShell grammar file) these tests will provide a degree of detection if breaking changes occur. * (maint) Modify tslint configuration for test files Previously tslint was raising errors in Travis CI saying that the excluded test fixtures directory was not included in the project. This was by design however it appears to be a known bug palantir/tslint#3793. This commit removes the exclude for test files from linting and adds a tslint directive in the default index.ts file. A tslint directive is used instead of solving the issue because this is the default testing file for VS Code extesions and shouldn't really be modified unless absolutely necessary. In this instance it was safer for a tslint directive. * (GH-1336) Add Code Folding settings and enable by default Previously the syntax folding was available for all users. However it was able to be configured or turned off. This commit adds a new `codeFolding` settings section, with the syntax folding feature enabled by default. * add copyright headers

This commit adds the the code to return folding ranges when the server receives a FoldingRangeRequest. This commit; * Uses a similar method to that implemented in the VS Code extension [1] * Uses the PowerShell tokeniser instead of the AST. This is due to the AST ignoring comment sections. Without the comment parsing folding for #region etc. will not work * Translates the tests in VS Code [1] into equivalent C# tests [1] PowerShell/vscode-powershell#1355

glennsarti force-pushed the add-syntax-folding branch from a339bc6 to 63650a0 Compare June 10, 2018 13:57

glennsarti force-pushed the add-syntax-folding branch from 38ea9ea to cd19f84 Compare June 13, 2018 14:01

glennsarti changed the title ~~WIP: (GH-1336) Add syntax aware folding provider~~ (GH-1336) Add syntax aware folding provider Jun 13, 2018

TylerLeonhardt requested review from rkeithhill, TylerLeonhardt, rjmholt and SeeminglyScience June 13, 2018 16:06

TylerLeonhardt reviewed Jun 13, 2018

View reviewed changes

rjmholt reviewed Jun 13, 2018

View reviewed changes

rjmholt mentioned this pull request Jun 13, 2018

Collapsible/expandable Functions, Regions, Comment blocks, and Comment based help blocks #1336

Closed

glennsarti force-pushed the add-syntax-folding branch from 7311340 to 4793314 Compare June 14, 2018 03:42

glennsarti added 7 commits June 29, 2018 10:25

(PowerShellGH-1336) Add syntax folding for braces and parentheses

09c8ade

This commit adds detection of text regions bounded by braces { } and parentheses ( ). This provides syntax aware folding for functions, arrays and hash tables.

(PowerShellGH-1336) Add syntax folding for here strings

f5ee6d2

This commit adds detection of text regions composed of single and double quoted here strings; @' '@ and @" "@.

(PowerShellGH-1336) Add syntax folding for comments

1e15180

This commit adds syntax aware folding for comment regions * Contiguous blocks of line comments `# ....` * Block comments `<# ... #>` * Region bound comments `# region ... # endregion`

(PowerShellGH-1336) Add Code Folding settings and enable by default

6a99426

Previously the syntax folding was available for all users. However it was able to be configured or turned off. This commit adds a new `codeFolding` settings section, with the syntax folding feature enabled by default.

glennsarti force-pushed the add-syntax-folding branch from 302c1c0 to 5156173 Compare June 29, 2018 02:27

rkeithhill reviewed Jun 29, 2018

View reviewed changes

rkeithhill approved these changes Jun 29, 2018

View reviewed changes

rjmholt approved these changes Jun 29, 2018

View reviewed changes

add copyright headers

d9a9e6d

glennsarti mentioned this pull request Jun 30, 2018

TSLint should enforce license headers #1394

Closed

TylerLeonhardt merged commit ef63eea into PowerShell:master Jul 2, 2018

glennsarti mentioned this pull request Nov 12, 2018

Move folding provider from Extension into Editor Services PowerShell/PowerShellEditorServices#793

Open

glennsarti mentioned this pull request Nov 13, 2018

(GH-793) Add syntax folding PowerShell/PowerShellEditorServices#777

Merged

(GH-1336) Add syntax aware folding provider #1355

(GH-1336) Add syntax aware folding provider #1355

Conversation

glennsarti commented Jun 9, 2018 • edited Loading

PR Summary

PR Checklist

glennsarti commented Jun 9, 2018

glennsarti commented Jun 10, 2018

rkeithhill commented Jun 10, 2018 • edited Loading

glennsarti commented Jun 11, 2018 • edited Loading

rkeithhill commented Jun 11, 2018 • edited Loading

glennsarti commented Jun 11, 2018

rkeithhill commented Jun 11, 2018

glennsarti commented Jun 11, 2018

rkeithhill commented Jun 11, 2018

glennsarti commented Jun 11, 2018

rkeithhill commented Jun 11, 2018

glennsarti commented Jun 11, 2018

TylerLeonhardt commented Jun 11, 2018

rjmholt commented Jun 11, 2018

glennsarti commented Jun 13, 2018

glennsarti commented Jun 13, 2018

TylerLeonhardt commented Jun 13, 2018

omniomi commented Jun 13, 2018

TylerLeonhardt left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt left a comment • 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

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

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

glennsarti Jun 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti commented Jun 14, 2018 • edited Loading

glennsarti commented Jun 29, 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

SeeminglyScience Jun 29, 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

glennsarti commented Jun 9, 2018 •

edited

Loading

rkeithhill commented Jun 10, 2018 •

edited

Loading

glennsarti commented Jun 11, 2018 •

edited

Loading

rkeithhill commented Jun 11, 2018 •

edited

Loading

rjmholt left a comment •

edited

Loading

glennsarti Jun 14, 2018 •

edited

Loading

glennsarti commented Jun 14, 2018 •

edited

Loading

SeeminglyScience Jun 29, 2018 •

edited

Loading