Remove redundant nil checks in RBI::Visitor
#312
Conversation
|
|
||
| sig { override.params(node: T.nilable(Node)).void } | ||
| def visit(node) | ||
| return unless node |
There was a problem hiding this comment.
Removing this causes a type failure before, even if I make Indexable a subtype of Node (by adding requires_ancestor { Node }):
lib/rbi/rewriters/merge_trees.rb:155: Expected RBI::Node but found T.all(RBI::Indexable, T.nilable(RBI::Node)) for argument right https://srb.help/7002
155 | make_conflict_tree(prev, node)
^^^^
Should T.all(RBI::Indexable, T.nilable(RBI::Node)) be simplified to T.all(RBI::Indexable, RBI::Node) (since it needs to be Indexable, it can't also be nil), and therefore be a subtype of RBI::Node?
|
|
||
| sig { override.params(node: T.nilable(Node)).void } | ||
| def visit(node) | ||
| return unless node |
There was a problem hiding this comment.
I'm not convinced removing these is a great idea. They're technically redundant, but they also fail fast on nil, which would be a really common case to hit (rather than doing 3 Class#=== checks below)
| # applies the ordering rules from the node_rank method as much as possible, while preserving visibility. | ||
| sorted_nodes = node.nodes.chunk do |n| | ||
| n.is_a?(Visibility) | ||
| end.flat_map do |_, nodes| |
There was a problem hiding this comment.
There's no Rubocop rule that can enforce this (last I checked), but having end.something_else do || on the same line is kinda weird, and reads better when it's one operation per line IMO.
| case node | ||
| when Tree |
There was a problem hiding this comment.
Removing these case statements with only one when worked out nicely.
1 participant
No description provided.