Allow skipping autocorrect from within autocorrect block #336
Conversation
| # Replaces the code of the given node with *content*. | ||
| def replace(node : Crystal::ASTNode, content) | ||
| replace(location(node), end_location(node), content) | ||
| end | ||
|
|
||
| # Inserts the given strings before and after the given node. | ||
| def wrap(node : Crystal::ASTNode, insert_before, insert_after) | ||
| wrap(location(node), end_location(node), insert_before, insert_after) | ||
| end | ||
|
|
||
| # Shortcut for `replace(node, "")` | ||
| def remove(node : Crystal::ASTNode) | ||
| remove(location(node), end_location(node)) | ||
| end | ||
|
|
||
| # Shortcut for `wrap(node, content, nil)` | ||
| def insert_before(node : Crystal::ASTNode, content) | ||
| insert_before(location(node), content) | ||
| end | ||
|
|
||
| # Shortcut for `wrap(node, nil, content)` | ||
| def insert_after(node : Crystal::ASTNode, content) | ||
| insert_after(end_location(node), content) | ||
| end | ||
|
|
||
| # Removes *size* characters prior to the given node. | ||
| def remove_preceding(node : Crystal::ASTNode, size) | ||
| remove_preceding(location(node), end_location(node), size) | ||
| end | ||
|
|
||
| # Removes *size* characters from the beginning of the given node. | ||
| # If *size* is greater than the size of the node, the removed region can | ||
| # overrun the end of the node. | ||
| def remove_leading(node : Crystal::ASTNode, size) | ||
| remove_leading(location(node), end_location(node), size) | ||
| end | ||
|
|
||
| # Removes *size* characters from the end of the given node. | ||
| # If *size* is greater than the size of the node, the removed region can | ||
| # overrun the beginning of the node. | ||
| def remove_trailing(node : Crystal::ASTNode, size) | ||
| remove_trailing(location(node), end_location(node), size) | ||
| end | ||
|
|
||
| private def location(node : Crystal::ASTNode) | ||
| node.location || raise "Missing location" | ||
| end | ||
|
|
||
| private def end_location(node : Crystal::ASTNode) | ||
| node.end_location || raise "Missing end location" | ||
| end | ||
|
|
There was a problem hiding this comment.
Why? Removing these will increase the amount of boilerplate code.
There was a problem hiding this comment.
One reason is that autocorrect probably should just be skipped if a node's location is missing (instead of raising an error). And while these methods could be updated to return early instead of raising an error, I think they would hide too much of what's going on behind the scenes. For example, the following autocorrect block might get only partially executed because name_location_end could be present while node.location or node.end_location is nil:
issue_for name_location, name_end_location, msg do |corrector|
next unless name_location_end = name_end_location(obj)
corrector.insert_after(name_location_end, '!')
corrector.remove_trailing(node, {{ ".not_nil!".size }})
endThere was a problem hiding this comment.
Yeah, I was thinking about this the other day. The optimal solution though would be IMO to let add_issue figure it out, instead of fiddling with it at the call-site.
There was a problem hiding this comment.
Mind explaining further? Don't know exactly what you mean
If an autocorrect block is exited without making any changes, then the issue in question should not be reported as "Correctable". On the one hand, this will reduce the maintenance burden of Ameba rules because
issue_forwill only need to be called once. On the other hand, this may negatively affect performance because eachIssuetemporarily creates an instance ofCorrectorto check if the issue is autocorrectable.