Better sorting for RBI nodes #405
Conversation
| node.nodes.sort! do |a, b| | ||
| return 0 if a.is_a?(Mixin) || b.is_a?(Mixin) |
There was a problem hiding this comment.
This was returning early. Thanks @paracycle 👀
| else | ||
| 9 |
There was a problem hiding this comment.
This was using the same rank number as scopes and consts
| res = node_rank(a) <=> node_rank(b) | ||
| res = node_name(a) <=> node_name(b) if res == 0 | ||
| res = (original_order[a] || 0) <=> (original_order[b] || 0) if res == 0 |
There was a problem hiding this comment.
Fall back on the original order if everything else fails
| @@ -11,11 +11,11 @@ class SortNodes < Visitor | |||
| def visit(node) | |||
| return unless node.is_a?(Tree) | |||
| visit_all(node.nodes) | |||
| original_order = original_nodes_order(node.nodes.dup) | |||
There was a problem hiding this comment.
Passing a copy so we always preserve the original order since the array will be sorted in place.
| when TStructField then 4 | ||
| when TEnumBlock then 5 | ||
| when Group then group_rank(node.kind) | ||
| when Include, Extend then 10 |
There was a problem hiding this comment.
Using 10, 20, etc. so we can add some sub-ordering when needed (example with Method a few lines below)
| res = node_rank(a) <=> node_rank(b) | ||
| res = node_name(a) <=> node_name(b) if res == 0 | ||
| res = (original_order[a] || 0) <=> (original_order[b] || 0) if res == 0 |
There was a problem hiding this comment.
I am not quite clear how we fallback to this case for mixins? Is it that their node names are always the same? What prevents mixin nodes from being sorted alphabetically?
There was a problem hiding this comment.
node_name always returns nil for nodes that shouldn't be sorted alphabetically: https://github.com/Shopify/tapioca/blob/master/lib/tapioca/rbi/rewriters/sort_nodes.rb#L69-L70
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Better sorting for RBI nodes
Motivation
Fix an early return in the node sorting as well as a conflicting node rank.
Implementation
Instead of returning early from the block, when two nodes are at a conflicting position in a tree, we preserve the original order in the non-sorted tree.
This means that mixins are always kept in the same order they were added to the tree even if moved around to satisfy the order against other node such as methods or constants.
Tests
See the new test file.