Skip to content

Break and return false in forced_brace_bounds? when Parent is a Binary #143

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

Conversation

weizheheng
Copy link

@weizheheng weizheheng commented Aug 21, 2022

Fixes #122

Signed-off-by: Wei Zhe Heng [email protected]

@weizheheng
Copy link
Author

weizheheng commented Aug 21, 2022

Thank you for this gem, I am enjoying it with the Neovim integration so far.

While going through the issue #122, it seems like the Binary node check should be added to #forced_brace_bounds? instead of #unchangeable_bounds?.

While adding a few tests, there is this test case that caught my attention:

Actual Behavior

  • Should the barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr.any? have the same indentation here?
foo =
  barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr if foooooooooooooooooooooo ||
  barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr.any? do |bar|
    bazzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
  end

What I expected

  • Extra indentation at the barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr.any?
foo =
  barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr if foooooooooooooooooooooo ||
    barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr.any? do |bar|
      bazzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
    end

Or should it be

if foooooooooooooooooooooo ||
     barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr.any? do |bar|
       bazzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
     end
  foo = barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr
end

@kddnewton
Copy link
Member

Thank you so much for the pull request! This is very detailed in the tests, which I appreciate.

I think it should be

if foooooooooooooooooooooo ||
     barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr.any? do |bar|
       bazzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
     end
  foo = barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr
end

at this point the modifier form of the if is too hard to read, so we should switch to multi-line.

@kddnewton
Copy link
Member

@weizheheng would you mind rebasing off main?

@weizheheng
Copy link
Author

@kddnewton Hi, yes of course, I'm on vacation now but will be back in three days. Will do it once I'm back.

@weizheheng weizheheng force-pushed the add-Binary-check-to-forced_brace_bounds branch from 3277d11 to 1372609 Compare October 23, 2022 08:31
@weizheheng
Copy link
Author

@kddnewton Done rebasing. This PR is purely fixing #122.

Will probably look into the below in a different PR.

Thank you so much for the pull request! This is very detailed in the tests, which I appreciate.

I think it should be

if foooooooooooooooooooooo ||
     barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr.any? do |bar|
       bazzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
     end
  foo = barrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr
end

at this point the modifier form of the if is too hard to read, so we should switch to multi-line.

@kddnewton
Copy link
Member

Thank you!

@kddnewton
Copy link
Member

@weizheheng looks like this failed CI on an idempotency check. You can check it locally with CI=true bundle exec rake. It checks the .rb files that ship with ruby itself.

@weizheheng weizheheng force-pushed the add-Binary-check-to-forced_brace_bounds branch 3 times, most recently from 13d1ec1 to 2196011 Compare October 25, 2022 02:49
@weizheheng weizheheng closed this Oct 25, 2022
@weizheheng weizheheng force-pushed the add-Binary-check-to-forced_brace_bounds branch from 2196011 to 78eea51 Compare October 25, 2022 02:54
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.

Block arguments in if statements uses {, triggering Rubocop
2 participants