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

Fix removing first method in class #538

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

skipkayhil
Copy link
Contributor

when following by a multi-line node.

Previously, trying to remove the first method in a module/class when there was another method after it would also remove all but the last line of the following method as well. The problem can be seen below:

module Foo
  def foo
  end
          # after.location.start_line - 1
  def bar # after.location.end_line - 1
  end
end

To account for blank lines, the end_line of the node to remove needs to be adjusted to include all lines upto the start of the next node, whereas the code was previously including all lines upto the end of the next node.

when following by a multi-line node.

Previously, trying to remove the first method in a module/class when
there was another method after it would also remove all but the last
line of the following method as well. The problem can be seen below:

```
module Foo
  def foo
  end
          # after.location.start_line - 1
  def bar # after.location.end_line - 1
  end
end
```

To account for blank lines, the end_line of the node to remove needs to
be adjusted to include all lines upto the _start_ of the next node,
whereas the code was previously including all lines upto the _end_ of
the next node.
@skipkayhil skipkayhil requested a review from a team as a code owner April 16, 2024 21:19
@skipkayhil skipkayhil requested review from Morriar and KaanOzkan April 16, 2024 21:19
@skipkayhil
Copy link
Contributor Author

I had an additional thought that many of the tests in this file use single line method/class definition, which could hide other bugs where a node's start_line == end_line. I wonder if some of them should be changed to use multi-line method/class definitions for better coverage of those cases.

Copy link
Collaborator

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

Good catch, thanks!

@Morriar Morriar merged commit ece6012 into Shopify:main Apr 17, 2024
6 checks passed
@skipkayhil skipkayhil deleted the hm-fix-rm-first-module-method branch April 22, 2024 22:57
@Morriar Morriar added the bugfix Fix a bug label May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fix a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants