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 RSpec/ExampleWording in escaping and % string literals #1800

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

r7kamura
Copy link
Contributor

@r7kamura r7kamura commented Feb 7, 2024

This change resolves some of the following Issue:

It still does not handle quotes well for dstr node, but at least it will handle them well for str node.


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).

If you have created a new cop:

  • Added the new cop to config/default.yml.
  • The cop is configured as Enabled: pending in config/default.yml.
  • The cop is configured as Enabled: true in .rubocop.yml.
  • The cop documents examples of good and bad code.
  • The tests assert both that bad code is reported and that good code is not reported.
  • Set VersionAdded: "<<next>>" in default/config.yml.

If you have modified an existing cop's configuration options:

  • Set VersionChanged: "<<next>>" in config/default.yml.

@r7kamura r7kamura requested a review from a team as a code owner February 7, 2024 00:01
@r7kamura
Copy link
Contributor Author

r7kamura commented Feb 7, 2024

Not related to this issue, but if string literals using %q, %, etc. are used, this cop may not be able to handle that well. It bothered me a little while reading the code. I haven't addressed it directly, but I have taken care to use node.loc.begin and node.loc.end insetad of ' or " so it will be somewhat better when we try to address it in the future.``

This cop has not been able to handle %(...) and other string literals that use %, I decided to take this opportunity to fix this problem as well.

Copy link
Member

@ydah ydah left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you.

@@ -106,6 +110,11 @@ def docstring(node)
)
end

def needs_escape?(node)
node.value.include?(node.loc.begin.source) ||
Copy link
Member

Choose a reason for hiding this comment

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

For quoted strings, both begin and end are the same. Do we need to check twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for %q(...), %Q(...), or %<...> etc.

irb(#<RuboCop::Cop::RSpec::Exampl...):001> node.loc.begin.source
=> "%q("
irb(#<RuboCop::Cop::RSpec::Exampl...):002> node.loc.end.source
=> ")"

But on second thought, we must check against node.loc.begin.source[-1], not node.loc.begin.source. I'll try to fix that.

Copy link
Contributor Author

@r7kamura r7kamura Feb 7, 2024

Choose a reason for hiding this comment

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

hmm, since escaping should only be necessary if begin and end are different (e.g. %!...! may require escaping like \!, while %(...) does not), it seems that only one of them needs to be checked after all.

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.

Thank you!

I’d love to extensively test this, but only the quoting autocorrection part. Wondering how to do that.

@r7kamura r7kamura force-pushed the example-wording-escape branch 2 times, most recently from 6b470ba to ffc6a8c Compare February 7, 2024 13:32
@r7kamura r7kamura changed the title Fix RSpec/ExampleWording autocorrection to correctly escape quotes on str node case Fix RSpec/ExampleWording in escaping and % string literals Feb 7, 2024
@r7kamura
Copy link
Contributor Author

r7kamura commented Feb 7, 2024

I've added a few more improvements:

  • changed #docstring for more accurate % string literals handling
  • removed unnecessary node.loc.begin checking from #needs_escape?
  • added test cases about %(...) and %!...!

Since the handling of % string literals has been improved and the scope of change is no longer only autocorrection, the title and changelog have been slightly changed accordingly.

@r7kamura r7kamura force-pushed the example-wording-escape branch 5 times, most recently from 35ffb03 to 2ef3761 Compare February 8, 2024 22:11
@@ -351,5 +351,95 @@
end
RUBY
end

context "when message includes `'` in `'...'`" do
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these examples are in the wrong context ('when DisallowedExamples: Workz').

RUBY

expect_correction(<<~RUBY)
it "returns foo's bar" do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it’s a silly question, but why are we changing the type of quotes from ' to " here? If we kept the same quote type, we wouldn’t need to concern ourselves with escaping, would we? (unless we insert an apostrophe/single quote)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for implementation convenience. If there is an implementation like string.inspect(quote_type: 'single_quote'), I would like to use it. If there is a way to rewrite only substrings while preserving quoting and escaping, that would be great, but do you have any other ideas?

Copy link
Contributor Author

@r7kamura r7kamura Feb 8, 2024

Choose a reason for hiding this comment

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

This is a just idea, but, if the offense range had been like this, we might not have to go through all this trouble?

it "should return foo's bar"
    ^^^^^^^^^^^^^

then we would only need to replace this range without changing the unrelated part that may contain troublesome
escapes.

Copy link
Member

Choose a reason for hiding this comment

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

Fantastic idea!

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.

4 participants