Implement VS Code syntax-based folding API #2360

rchande · 2018-06-01T19:57:31Z

akshita31 · 2018-06-04T19:14:52Z

src/features/structureProvider.ts

+import { blockStructure } from "../omnisharp/utils";
+import { Request } from "../omnisharp/protocol";
+
+export class structureProvider extends AbstractSupport implements FoldingRangeProvider


Should this be in capital ?

Will fix. Also--it looks like many other classes start with Capital letters but have filenames that start with lowercase letters. Should we try to make this consistent or is this some typescriptism?

DustinCampbell · 2018-06-05T17:35:52Z

src/features/structureProvider.ts

+import { blockStructure } from "../omnisharp/utils";
+import { Request } from "../omnisharp/protocol";
+
+export class structureProvider extends AbstractSupport implements FoldingRangeProvider


DustinCampbell · 2018-06-05T17:37:41Z

src/omnisharp/protocol.ts

+        HintSpan: Range;
+        BannerText: string;
+        Type: string;
+    }


This protocol should match other OmniSharp endpoints better. It'd probably better to have just a single Range property rather than all of the other properties that are ultimately unused.

BlockStructureRequest appears to be missing

@DustinCampbell This is something the extension codebase is inconsistent about: for request types that derive Request but don't add any properties, we haven't been adding corresponding entries in protocol.ts (instead just preferring to use Request directly) (case in point: there's no FileMembersAsTree request type in the extension). I was following that pattern here. We could also switch to adding request-deriving types that don't add properties. What do you think?

codecov · 2018-06-06T17:12:55Z

Codecov Report

Merging #2360 into master will increase coverage by 0.3%.
The diff coverage is 85.71%.

@@            Coverage Diff            @@
##           master    #2360     +/-   ##
=========================================
+ Coverage   62.25%   62.56%   +0.3%     
=========================================
  Files          86       87      +1     
  Lines        3855     3876     +21     
  Branches      551      552      +1     
=========================================
+ Hits         2400     2425     +25     
+ Misses       1293     1287      -6     
- Partials      162      164      +2

Flag	Coverage Δ
#integration	`51.71% <85.71%> (+0.36%)`	⬆️
#unit	`84.26% <100%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
src/omnisharp/extension.ts	`80.18% <100%> (+0.38%)`	⬆️
src/omnisharp/utils.ts	`70.17% <100%> (+1.08%)`	⬆️
src/omnisharp/protocol.ts	`78.88% <100%> (+0.23%)`	⬆️
src/features/structureProvider.ts	`81.25% <81.25%> (ø)`
src/omnisharp/delayTracker.ts	`68.42% <0%> (ø)`	⬆️
src/assets.ts	`50% <0%> (+2.91%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2bed4f...078ed96. Read the comment docs.

DustinCampbell

tests?

DustinCampbell · 2018-06-07T15:23:27Z

src/omnisharp/launcher.ts

@@ -228,6 +228,7 @@ async function launch(cwd: string, args: string[], launchInfo: LaunchInfo, platf
        args.push(`formattingOptions:useTabs=${!getConfigurationValue(globalConfig, csharpConfig, 'editor.insertSpaces', true)}`);
        args.push(`formattingOptions:tabSize=${getConfigurationValue(globalConfig, csharpConfig, 'editor.tabSize', 4)}`);
        args.push(`formattingOptions:indentationSize=${getConfigurationValue(globalConfig, csharpConfig, 'editor.tabSize', 4)}`);
+        args.push('-z')


I don't think you want this. If you launch OmniSharp with --zero-based-indeces, you'll need to update all of the places in C# for VS Code that expect 1-based indeces today.

DustinCampbell · 2018-06-07T15:25:32Z

src/omnisharp/protocol.ts

+    export interface BlockSpan {
+        Range: Range;
+        Type: string;
+    }


I think you're missing your BlockStructureRequest type.

DustinCampbell · 2018-06-07T15:26:31Z

src/omnisharp/utils.ts

@@ -19,6 +19,10 @@ export async function currentFileMembersAsTree(server: OmniSharpServer, request:
    return server.makeRequest<protocol.CurrentFileMembersAsTreeResponse>(protocol.Requests.CurrentFileMembersAsTree, request, token);
 }

+export async function blockStructure(server: OmniSharpServer, request: protocol.Request, token: vscode.CancellationToken) {


In the protocol you defined on the OmniSharp side, this takes a BlockStructureRequest, which inherits from SimpleFileRequest rather than Request.

DustinCampbell · 2018-06-07T15:27:19Z

src/features/structureProvider.ts

+        else if (type == "Region")
+        {
+            return FoldingRangeKind.Region
+        }


DustinCampbell · 2018-06-07T15:27:52Z

src/features/structureProvider.ts

+        }
+
+        let response = await blockStructure(this._server, request, token)
+        let ranges : FoldingRange[] = [];


nit: format this file? This colon is in a different spot than it is above.

DustinCampbell · 2018-06-07T15:28:00Z

src/features/structureProvider.ts

+        return ranges;
+    }
+
+    GetType(type: string) : FoldingRangeKind


…into folding

DustinCampbell · 2018-07-09T20:47:48Z

When will we update OmniSharp in C# for VS Code so that this works without specifying "omnisharp.path": "latest"? Technically, master is broken (at least for this feature) with the default OmniSharp that is specified in master.

rchande · 2018-07-10T17:03:23Z

@DustinCampbell let's try to do it this week--I've requested opinions in the o# slack.

Implement VS Code syntax-based folding API

ea335f9

rchande requested review from TheRealPiotrP and akshita31 June 1, 2018 19:57

akshita31 reviewed Jun 4, 2018

View reviewed changes

akshita31 approved these changes Jun 4, 2018

View reviewed changes

DustinCampbell added the Blocked on OmniSharp label Jun 5, 2018

DustinCampbell reviewed Jun 5, 2018

View reviewed changes

CR feedback

66fbcb4

DustinCampbell reviewed Jun 7, 2018

View reviewed changes

Ravi Chande added 8 commits June 26, 2018 14:24

Merge branch 'master' of https://github.com/omnisharp/omnisharp-vscode …

0be92cd

…into folding

CR feedback

02c7068

Merge branch 'master' into folding

995749b

Fix typo in settings.json

64b981d

Merge branch 'folding' of https://github.com/rchande/omnisharp-vscode …

50c1aa8

…into folding

update vscode version in travis

70dc9ec

Fix package.json

74c7365

Add blockstructurerequest type

078ed96

rchande mentioned this pull request Jun 27, 2018

Preselect feature in the completion item #2388

Merged

rchande merged commit 6d115ca into dotnet:master Jun 27, 2018

rchande deleted the folding branch June 27, 2018 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement VS Code syntax-based folding API #2360

Implement VS Code syntax-based folding API #2360

rchande commented Jun 1, 2018

akshita31 Jun 4, 2018

DustinCampbell Jun 5, 2018

rchande Jun 6, 2018

DustinCampbell Jun 5, 2018

DustinCampbell Jun 5, 2018

DustinCampbell Jun 5, 2018

rchande Jun 6, 2018

codecov bot commented Jun 6, 2018 •

edited

Loading

DustinCampbell left a comment

DustinCampbell Jun 7, 2018

DustinCampbell Jun 7, 2018

DustinCampbell Jun 7, 2018

DustinCampbell Jun 7, 2018

DustinCampbell Jun 7, 2018

DustinCampbell Jun 7, 2018

DustinCampbell commented Jul 9, 2018 •

edited

Loading

rchande commented Jul 10, 2018

Implement VS Code syntax-based folding API #2360

Implement VS Code syntax-based folding API #2360

Conversation

rchande commented Jun 1, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 6, 2018 • edited Loading

Codecov Report

DustinCampbell 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

DustinCampbell commented Jul 9, 2018 • edited Loading

rchande commented Jul 10, 2018

codecov bot commented Jun 6, 2018 •

edited

Loading

DustinCampbell commented Jul 9, 2018 •

edited

Loading