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

Do not register an offense for UnneededPercentQ when %q or %Q occurs in an interpolated string #2274

Merged
merged 3 commits into from Oct 5, 2015

Conversation

rrosenblum
Copy link
Contributor

I came across a strange issue with UnneededPercentQ earlier. It would register an offense for a double quoted string that uses interpolation and has a section that begins with %q or %Q.

example offenses

"%q#{foo}"
"%Q#{foo}"
"#{foo}%q"
"#{foo}%Q"

@@ -14,6 +14,7 @@ def on_dstr(node)
end

def on_str(node)
return unless standalone_string?(node)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, that's odd. A string with interpolation is actually a dstr and we have a check for it in on_dstr. I guess we can do more or less the same check here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was surprised by this as well. Given the example string "%q(a)#{b}", this cop was called twice. Once for dstr with "%q(a)#{b}", and a second time for str with the substring "%q(a)". It seemed weird to me that this was the behavior. That is why I had to add in the check to see if the string is a substring.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like some parser bug. You should check the generated AST.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Scratch this, it's not really a bug of course.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll think about this a bit more - at the very least standalone_string? should be string_literal? and there should probably be some explanation about why this check is needed. The cop could use a better class comment in general. Probably using the same check method for both cases wasn't a great idea either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will make the change to the method name and add comments. I will also look into refactoring this cop to implement different checks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great.

@rrosenblum rrosenblum force-pushed the fix_unneeded_percent_q branch 4 times, most recently from 609913e to 1d6246c Compare October 2, 2015 21:48
@rrosenblum rrosenblum changed the title Do not register an offense for UnneededPercentQ when %q or %Q occurs in an interpolated string WIP - Do not register an offense for UnneededPercentQ when %q or %Q occurs in an interpolated string Oct 2, 2015
@rrosenblum rrosenblum force-pushed the fix_unneeded_percent_q branch 4 times, most recently from 8755ea1 to 1be4572 Compare October 3, 2015 22:50
@rrosenblum
Copy link
Contributor Author

I think that the source code is about as clean as it is going to get. It turns out that my fix also handles the case for regex. I tried to extract the code for dstr and str but the necessary code was nearly identical. Let me know what you think of the changes.

@rrosenblum rrosenblum changed the title WIP - Do not register an offense for UnneededPercentQ when %q or %Q occurs in an interpolated string Do not register an offense for UnneededPercentQ when %q or %Q occurs in an interpolated string Oct 5, 2015
bbatsov added a commit that referenced this pull request Oct 5, 2015
Do not register an offense for UnneededPercentQ when %q or %Q occurs in an interpolated string
@bbatsov bbatsov merged commit 0ad7df5 into rubocop:master Oct 5, 2015
@bbatsov
Copy link
Collaborator

bbatsov commented Oct 5, 2015

Looks OK to me.

@rrosenblum rrosenblum deleted the fix_unneeded_percent_q branch October 5, 2015 14:57
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

2 participants