Navigation bar class expression #8648
Conversation
|
Hi @Andy-MS, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
ghost
force-pushed
the
navigation_bar_class_expression
branch
from
e39352b to
2a67c6e
Compare
|
|
||
| default: | ||
| const childrens: Node[] = []; | ||
| forEachChild(node, child => { childrens.push(child) }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
we can do a deep sort on the result once at the end, would that work?
ghost
force-pushed
the
navigation_bar_class_expression
branch
2 times, most recently
from
e8c1e94 to
61f8e99
Compare
| function addTopLevelNodes(nodes: Node[], higherLevel: Node[]): void { | ||
| const thisLevel: Node[] = []; | ||
| for (let node of nodes) | ||
| addTopLevelNode(node, thisLevel); |
There was a problem hiding this comment.
We always use curlies in these constructs. Could you add this file to the lint sources in Jakefile.js?
ghost
force-pushed
the
navigation_bar_class_expression
branch
2 times, most recently
from
76d341b to
ecfac58
Compare
|
👍 |
ghost
force-pushed
the
navigation_bar_class_expression
branch
3 times, most recently
from
5a0de9b to
42d15d1
Compare
ghost
force-pushed
the
navigation_bar_class_expression
branch
from
42d15d1 to
f8acf11
Compare
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
force-pushed
the
navigation_bar_class_expression
branch
from
f8acf11 to
c9ec628
Compare
|
This should wait on #8811 so we can add |
|
This should probably also wait on #8812. |
| } | ||
| } | ||
|
|
||
| function isAnonFn(item: NavigationBarItem): boolean { |
There was a problem hiding this comment.
Please spell names out, so anonymousFunctionText, isAnonymousFunction, and anonymousClassText
| const anonFnText = "<function>"; | ||
| const anonClassText = "<class>"; | ||
|
|
||
| // Get the name for a (possibly anonymous) class/function expression. |
ghost
mentioned this pull request
|
Closed in favor of #8958 |
Fixes #5258
Should wait for #8622.