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

Change the start label of a heredocument to be enclosed in single quotes #1543

Merged
merged 1 commit into from
Jan 10, 2023

Conversation

ydah
Copy link
Member

@ydah ydah commented Jan 10, 2023

This PR is change as follows

# bad - This would require escaping inside heredoc. 
expect_offense(<<-RUBY)
  it 'should do something ' \\ <---here
  ^^^^^^^^^^^^^^^^^^^^^^^ Do not use should when describing your tests.
    'and correctly fix' do
  end
RUBY

# good - This makes the heredoc readable with no need for escaping inside it.
expect_offense(<<-'RUBY')
  it 'should do something ' \
  ^^^^^^^^^^^^^^^^^^^^^^^ Do not use should when describing your tests.
    'and correctly fix' do
  end
RUBY

Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • [-] Updated documentation.
  • [-] Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

context 'the display name not present' do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Context description should match /^when\b/, /^with\b/, or /^without\b/.
Copy link
Member

Choose a reason for hiding this comment

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

There is a difference at a glance. Wondering how it turns out that both work 🤔

<<~A
/^when\b/
A
=> "/^when\b/\n"
<<~'A'
/^when\b/
A
=> "/^when\\b/\n"

Maybe there's some escaping logic in expect_offense? I can't find any code in expect_offense and there are no specs for this method or AnnotatedSource in rubocop specs.

Copy link
Member Author

Choose a reason for hiding this comment

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

The argument of expect_offense is a heap document, so a heap document enclosed in single quotes is completely a string as written. So the following would be the same.

irb(main):001:0" <<~A
irb(main):002:0" /^when\\b/
irb(main):003:0> A
=> "/^when\\b/\n"
irb(main):004:0' <<~'A'
irb(main):005:0' /^when\b/
irb(main):006:0> A
=> "/^when\b/\n"

Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, please accept my apologies, I've missed the point that you've changed the contents of the string and removed unnecessary escaping.
Looks much better this way indeed!

Copy link
Member Author

Choose a reason for hiding this comment

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

The title of this PR was misleading. My apologies.
Thanks for the review!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add to the PR description what the benefit/downside is of either syntax. I always forget which is which.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for review. I updated description.

Comment on lines +124 to 126
describe '#mymethod ' \
'(is cool)' do
end
Copy link
Member

Choose a reason for hiding this comment

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

<<-A
a \
b
A
=> "a b\n"

The \ part is lost, and that undermines the very existence of this example which is purposed to test "-separated multiline strings".

Copy link
Member Author

Choose a reason for hiding this comment

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

The results seem to be the same here as well.

irb(main):001:0" <<-A
irb(main):002:0" a \\
irb(main):003:0" b
irb(main):004:0> A
=> "a \\\nb\n"
irb(main):005:0' <<-'A'
irb(main):006:0' a \
irb(main):007:0' b
irb(main):008:0> A
=> "a \\\nb\n"

@ydah ydah requested a review from pirj January 10, 2023 11:19
This PR is change as follows

```ruby
expect_offense(<<-RUBY)
  # ...
end

expect_offense(<<-'RUBY')
  # ...
end
```
@ydah ydah force-pushed the remove_unnecessary_escapes branch from deace2e to aa0a9e3 Compare January 10, 2023 11:22
@ydah ydah changed the title Remove unnecessary escapes in heredoc Change the start label of a heredocument to be enclosed in single quotes Jan 10, 2023
@ydah
Copy link
Member Author

ydah commented Jan 10, 2023

The title of the PR was incorrect and has been corrected.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Much easier to read now, thank you!

@pirj pirj merged commit a1ece2c into rubocop:master Jan 10, 2023
@ydah ydah deleted the remove_unnecessary_escapes branch January 10, 2023 13:46
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.

3 participants