Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow skipping autocorrect from within autocorrect block #336

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FnControlOption
Copy link
Contributor

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_for will only need to be called once. On the other hand, this may negatively affect performance because each Issue temporarily creates an instance of Corrector to check if the issue is autocorrectable.

Comment on lines -143 to -194
# 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Removing these will increase the amount of boilerplate code.

Copy link
Contributor Author

@FnControlOption FnControlOption Dec 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }})
end

Copy link
Member

@Sija Sija Dec 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind explaining further? Don't know exactly what you mean

@Sija Sija added this to the 1.5.0 milestone Dec 23, 2022
@Sija Sija removed this from the 1.5.0 milestone Feb 19, 2023
@Sija Sija removed the rule label Nov 14, 2023
@Sija Sija marked this pull request as draft April 29, 2024 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants