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

Update RedundantExtendTSig to trim whole line #172

Merged
merged 1 commit into from Aug 16, 2023

Conversation

bitwise-aiden
Copy link
Contributor

Description

When running this cop it will leave whatever whitespace occurred at the start of the line, which means that if run in isolation may cause other violations.

This approach is in line with what occurs in ForbidExtendTSigHelpersInShims, replacing the whole line and removing any new line.

The thing to consider is that it will also remove any comment on the line, but this feels like a fine tradeoff since it would likely be to do with the extend in the first place and would need to be cleaned up.

@bitwise-aiden bitwise-aiden requested a review from a team as a code owner August 14, 2023 14:07
@@ -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.

@bitwise-aiden bitwise-aiden force-pushed the bitwise-aiden/update-redundant-extend-cop branch from 79f504b to 25dbea5 Compare August 15, 2023 16:51
@bitwise-aiden bitwise-aiden force-pushed the bitwise-aiden/update-redundant-extend-cop branch from 25dbea5 to 5094122 Compare August 15, 2023 17:30
@bitwise-aiden bitwise-aiden merged commit c84fcf2 into main Aug 16, 2023
10 checks passed
@bitwise-aiden bitwise-aiden deleted the bitwise-aiden/update-redundant-extend-cop branch August 16, 2023 20:22
@shopify-shipit shopify-shipit bot temporarily deployed to production September 25, 2023 14:39 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants