-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Add new Performance/UnfreezeString
cop
#4586
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
# @example | ||
# # bad | ||
# "string".dup | ||
# String.new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to add String.new("")
to a bad case. It takes an empty string as an argument.
# @example | ||
# # bad | ||
# "string".dup | ||
# String.new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this isn't actually checked by the cop. 🙂
module RuboCop | ||
module Cop | ||
module Performance | ||
# In Ruby 2.3 or later, Use unary plus operator to unfreeze a string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Use" with lowercase "u", since it's in the middle of a sentence. 😊
|
||
it 'registers an offense for a heredoc with `.dup`' do | ||
expect_offense(<<-RUBY.strip_indent) | ||
<<TEXT.dup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Where does the plus go here? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+<<TEXT
foo
bar
TEXT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Had no idea this worked. 🙂
# @example | ||
# # bad | ||
# "string".dup | ||
# String.new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I'll add String.new('')
to bad case.
And I'll add checks for String.new
, String.new('')
and String.new('something')
.
b015d63
to
1dacee1
Compare
I added checks for |
In Ruby 2.3 or later, `String#+@` is available. This method unfreezes a string. ```ruby str = 'foo'.freeze p str.frozen? # => true p (+str).frozen? # => false ``` `String#dup` works similarly, but `+@` is faster than `dup`. See. https://gist.github.com/k0kubun/e3da77cae2c132badd386c96f2de5768 This cop recommends to use `+@` instead of `dup`.
|
||
def_node_matcher :string_new?, <<-PATTERN | ||
{ | ||
(send (const nil :String) :new {str dstr}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good reminder that I should look at adding an "optional" operator. 😅
In Ruby 2.3 or later,
String#+@
is available.This method unfreezes a string.
String#dup
works similarly, but+@
is faster thandup
.See. https://gist.github.com/k0kubun/e3da77cae2c132badd386c96f2de5768
This cop recommends to use
+@
instead ofdup
.Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).rake spec
) are passing.rake internal_investigation
.and description in grammatically correct, complete sentences.
rake generate_cops_documentation
(required only when you've added a new cop or changed the configuration/documentation of an existing cop).