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.

Sign up for GitHub

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

Jump to bottom

Update RedundantExtendTSig to trim whole line #172

Merged
merged 1 commit into from
Aug 16, 2023
Merged

Update RedundantExtendTSig to trim whole line #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/rubocop/cop/sorbet/redundant_extend_t_sig.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ module Sorbet
# end
#
class RedundantExtendTSig < RuboCop::Cop::Base
include RangeHelp
extend AutoCorrector

MSG = "Do not redundantly `extend T::Sig` when it is already included in all modules."
Expand All @@ -40,7 +41,7 @@ def on_send(node)
return unless extend_t_sig?(node)

add_offense(node) do |corrector|
corrector.remove(node)
corrector.remove(range_by_whole_lines(node.source_range, include_final_newline: true))
Copy link
Member

Choose a reason for hiding this comment

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

What if we did range_with_surrounding_space(range: node.source_range) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a lot cleaner. I'm guessing this will also take out additional newlines before and after?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, that could be why
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the docs, I wonder how many cases would be solved with:

range_with_surrounding_space(range: node.source_range, side: :right)

It feels like :both will take take too many lines, but I can see a case for extend T::Sig having trailing new lines. Absolutely not substantiated by anything but I think it would cover all these cases:

class SomeClass 
  extend T::Sig
end 

class SomeClass
end
class SomeClass 
  extendT::Sig

  def initialize; end
end

class SomeClass
  def initialize; end
end
class SomeClass
  include SomeHelperModule

  extend T::Sig

  def initialize; end
end

class SomeClass
  include SomeHelperModule

  def initialize; end
end

Writing those out has convinced me that it will likely yield the best results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, not quite! This one is wrong:

class SomeClass 
  extend T::Sig
end 

class SomeClass
  end

The indentation is kept because it doesn't go left. Though that becomes tough to manage since if it was anything other than end we would be fine with it.

Maybe it is best to keep it how it was and rely on the other cops to ensure the lines are cleaned up.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, what if we did: range_with_surrounding_space(range: node.source_range, side: :right) and joined it with range_with_surrounding_space(range: node.source_range, side: :left, newlines: false). So:

add_range(
  range_with_surrounding_space(range: node.source_range, side: :left, newlines: false),
  range_with_surrounding_space(range: node.source_range, side: :right)
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would then fail on:

class SomeClass
  extend T::Sig

  def initialize; end
end

class SomeClass
def initialize; end
end

It would be nice if there was a way to consume upto the next line that has content on it.

Copy link
Member

Choose a reason for hiding this comment

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

ok, fair. I do agree that what we have is the best compromise. thanks for indulging my questions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the inquiry, it's good to know what is going on at a deeper level.

end
end
end
Expand Down
13 changes: 3 additions & 10 deletions spec/rubocop/cop/sorbet/redundant_extend_t_sig_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

expect_correction(<<~RUBY)
#{header}
#{trailing_whitespace}
end
RUBY
end
Expand Down Expand Up @@ -43,7 +42,7 @@ module M
^^^^^^^^^^^^^ #{message}
RUBY

expect_correction(a_blank_line)
expect_correction("")
end

it "registers an offense when using `extend ::T::Sig` (fully qualified)" do
Expand All @@ -52,7 +51,7 @@ module M
^^^^^^^^^^^^^^^ #{message}
RUBY

expect_correction(a_blank_line)
expect_correction("")
end

it "registers an offense when using `extend T::Sig` with an explicit receiver" do
Expand All @@ -61,7 +60,7 @@ module M
^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
RUBY

expect_correction(a_blank_line)
expect_correction("")
end

it "does not register an offense when extending other modules in the T namespace" do
Expand All @@ -71,10 +70,4 @@ module M
end
RUBY
end

private

def a_blank_line
"\n"
end
end