Navigation bar class expression · Pull Request #8648 · microsoft/TypeScript

ghost · 2016-05-17T17:42:33Z

Fixes #5258
Should wait for #8622.

msftclas · 2016-05-17T17:42:37Z

Hi @Andy-MS, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Andy Hanson). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

mhegazy · 2016-05-17T17:47:26Z

src/services/navigationBar.ts

+
+                    default:
+                        const childrens: Node[] = [];
+                        forEachChild(node, child => { childrens.push(child) });


We should change this function to take a node instead of a list. the allocations here are not really needed. we put nodes in a new array just to call the function, then throw them away.

It does sort the nodes first. Although it looks like it would be equivalent to add nodes in arbitrary order and sort topLevelNodes at the end.

i woudl split this into two functions, addTopLevelNodes:

function addTopLevelNodes(nodes: Node[], topLevelNodes: Node[]): void { forEach(nodes, addTopLevelNode); }

Now the next issue is to figure out what to do with the sorting. i believe we can sort the output instead.

Although it looks like it would be equivalent to add nodes in arbitrary order and sort topLevelNodes at the end.

I think they are the same.

Currently it sorts within a single level, so e.g. methods of a class will be sorted just among each other. For levels of expressions we probably don't want that (foo(foo(class Y {}, class X {}), foo(class B {}, class A{})) should show as A B X Y).

we can do a deep sort on the result once at the end, would that work?

DanielRosenwasser · 2016-05-18T07:11:24Z

src/services/navigationBar.ts

+        function addTopLevelNodes(nodes: Node[], higherLevel: Node[]): void {
+            const thisLevel: Node[] = [];
+            for (let node of nodes)
+                addTopLevelNode(node, thisLevel);


We always use curlies in these constructs. Could you add this file to the lint sources in Jakefile.js?

mhegazy · 2016-05-18T17:44:34Z

👍

Previous algorithm would sort *after* adding to top-level nodes. This was broken because top-level nodes were simply all in a flat array, so this would cause sorting among unrelated elements. Now we collect all the nodes in a single logical level and sort them before adding them to topLevelNodes.

ghost · 2016-05-25T15:04:36Z

This should wait on #8811 so we can add SyntaxKind.ClassExpression to the case there.

ghost · 2016-05-26T15:43:25Z

This should probably also wait on #8812.

mhegazy · 2016-05-26T22:52:49Z

src/services/navigationBar.ts

+        }
+    }
+
+    function isAnonFn(item: NavigationBarItem): boolean {


Please spell names out, so anonymousFunctionText, isAnonymousFunction, and anonymousClassText

mhegazy · 2016-05-26T22:55:19Z

src/services/navigationBar.ts

+    const anonFnText = "<function>";
+    const anonClassText = "<class>";
+
+    // Get the name for a (possibly anonymous) class/function expression.


jsdoc comments

…ed a const instead of a function

ghost · 2016-06-03T15:28:08Z

Closed in favor of #8958

msftclas added the cla-not-required label May 17, 2016

ghost force-pushed the navigation_bar_class_expression branch from e39352b to 2a67c6e Compare May 17, 2016 17:46

mhegazy reviewed May 17, 2016
View reviewed changes

ghost force-pushed the navigation_bar_class_expression branch 2 times, most recently from e8c1e94 to 61f8e99 Compare May 17, 2016 19:39

DanielRosenwasser reviewed May 18, 2016
View reviewed changes

ghost force-pushed the navigation_bar_class_expression branch 2 times, most recently from 76d341b to ecfac58 Compare May 18, 2016 17:19

ghost force-pushed the navigation_bar_class_expression branch 3 times, most recently from 5a0de9b to 42d15d1 Compare May 20, 2016 21:07

Andy Hanson added 3 commits May 23, 2016 06:12

Add class expressions to navigation bar

d80677b

Fix sort order for toplevel declarations in nested expressions

86cce79

Respond to PR comments

dda27f7

ghost force-pushed the navigation_bar_class_expression branch from 42d15d1 to f8acf11 Compare May 23, 2016 13:12

ghost force-pushed the navigation_bar_class_expression branch from f8acf11 to c9ec628 Compare May 23, 2016 13:12

Merge branch 'master' into navigation_bar_class_expression

d362e6a

ghost assigned mhegazy May 24, 2016

mhegazy reviewed May 26, 2016
View reviewed changes

Andy Hanson added 4 commits May 27, 2016 06:21

Improve naming

1d71bf7

Merge branch 'master' into navigation_bar_class_expression

c21648d

Update tests and fix bug where const x = function() {} was consider…

a7bedb1

…ed a const instead of a function

Fix bug where class expression child item didn't get right name

1fc1813

ghost mentioned this pull request May 27, 2016

Navbar refactor #8865

Closed

ghost closed this Jun 3, 2016

ghost mentioned this pull request Jun 13, 2016

Refactor navigation bar #8958

Closed

mhegazy deleted the navigation_bar_class_expression branch November 2, 2017 21:02

microsoft locked and limited conversation to collaborators Jun 19, 2018

This pull request was closed.

Conversation

ghost commented May 17, 2016

Uh oh!

msftclas commented May 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy commented May 18, 2016

Uh oh!

ghost commented May 25, 2016

Uh oh!

ghost commented May 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants