- Notifications
You must be signed in to change notification settings - Fork9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Remove redundant nil checks inRBI::Visitor#312
base:main
Are you sure you want to change the base?
Conversation
| @@ -117,8 +117,6 @@ def initialize(output, left_name: "left", right_name: "right", keep: Keep::NONE) | |||
| sig { override.params(node: T.nilable(Node)).void } | |||
| def visit(node) | |||
| return unless node | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Removing this causes a type failure before, even if I makeIndexable a subtype ofNode (by addingrequires_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) ^^^^ShouldT.all(RBI::Indexable, T.nilable(RBI::Node)) be simplified toT.all(RBI::Indexable, RBI::Node) (since it needs to be Indexable, it can't also be nil), and therefore be a subtype ofRBI::Node?
| @@ -81,8 +81,6 @@ def visit_all(nodes) | |||
| sig { override.params(node: T.nilable(Node)).void } | |||
| def visit(node) | |||
| return unless node | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not convinced removing these is a great idea. They'retechnically redundant, but they also fail fast onnil, which would be a really common case to hit (rather than doing 3Class#=== checks below)
| # The child nodes could contain private/protected markers. If so, they should not be moved in the file. | ||
| # Otherwise, some methods could see their privacy change. To avoid that problem, divide the array of child | ||
| # nodes into chunks based on whether any Visibility nodes appear, and sort the chunks independently. This | ||
| # 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There's no Rubocop rule that can enforce this (last I checked), but havingend.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Removing thesecase statements with only onewhen worked out nicely.
No description provided.