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 Ruby 2.7 warnings #38

Merged
merged 2 commits into from
Oct 11, 2020
Merged

Conversation

kikihakiem
Copy link

This PR fixes #35

mock_attachment = MockAttachment.new(options)
url_generator = Paperclip::UrlGenerator.new(mock_attachment)

expect { url_generator.for(:style_name, escape: true) }.to_not(output(/URI\.(un)?escape is obsolete/).to_stderr)
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [116/80]

@@ -194,6 +194,16 @@ def escape
"expected the interpolator to be passed #{expected.inspect} but it wasn't"
end

it "doesn't emit deprecation warnings" do
expected = "the expected result"
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

end

it "doesn't emit deprecation warnings" do
expect { subject }.to_not(output(/URI\.(un)?escape is obsolete/).to_stderr)
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [81/80]

@subject.original_filename = "image.png"
assert_equal "image.png", @subject.original_filename
subject.original_filename = "image.png"
expect(subject.original_filename).to(eq("image.png"))
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

end

it "accepts an original_filename" do
@subject.original_filename = "image.png"
assert_equal "image.png", @subject.original_filename
subject.original_filename = "image.png"
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

end

it "returns a content type" do
assert_equal "image/png", @subject.content_type
expect(subject.content_type).to(eq("image/png"))
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

end

it "closes open handle after reading" do
assert_equal true, @open_return.closed?
expect { subject }.to(change { @open_return.closed? }.from(false).to(true))
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [81/80]

end

it "returns a file name" do
assert_equal "thoughtbot-logo.png", @subject.original_filename
expect(subject.original_filename).to(eq("thoughtbot-logo.png"))
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

@@ -55,7 +55,7 @@ def attachment(*attachment_names)
attachment_names.each do |attachment_name|
COLUMNS.each_pair do |column_name, column_type|
column_options = options.merge(options[column_name.to_sym] || {})
column("#{attachment_name}_#{column_name}", column_type, column_options)
column("#{attachment_name}_#{column_name}", column_type, **column_options)
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [86/80]

@@ -26,7 +26,7 @@ def add_attachment(table_name, *attachment_names)
attachment_names.each do |attachment_name|
COLUMNS.each_pair do |column_name, column_type|
column_options = options.merge(options[column_name.to_sym] || {})
add_column(table_name, "#{attachment_name}_#{column_name}", column_type, column_options)
add_column(table_name, "#{attachment_name}_#{column_name}", column_type, **column_options)
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [102/80]

@@ -28,7 +28,7 @@ def reset_class(class_name)

def reset_table(_table_name, &block)
block ||= lambda { |_table| true }
ActiveRecord::Base.connection.create_table :dummies, { force: true }, &block
ActiveRecord::Base.connection.create_table :dummies, **{ force: true }, &block
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [82/80]

@kikihakiem kikihakiem changed the title Fix Ruby 2.7.1 warnings Fix Ruby 2.7 warnings Sep 27, 2020
@kikihakiem kikihakiem force-pushed the fix-ruby271-warnings branch 2 times, most recently from 2852113 to 03b1097 Compare September 28, 2020 19:45
@@ -53,7 +53,7 @@ def default_filename
def download_content
options = { read_timeout: Paperclip.options[:read_timeout] }.compact

open(@target, **options)
self.open(@target, options)
Copy link

Choose a reason for hiding this comment

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

Style/RedundantSelf: Redundant self detected.

@@ -65,7 +72,7 @@ def escape_url(url)
if url.respond_to?(:escape)
url.escape
else
URI.escape(url).gsub(escape_regex) { |m| "%#{m.ord.to_s(16).upcase}" }
self.class.escape(url).gsub(escape_regex) { |m| "%#{m.ord.to_s(16).upcase}" }
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [85/80]

escaped = URI.escape(target)
super(URI(target == URI.unescape(target) ? escaped : target), options)
escaped = Paperclip::UrlGenerator.escape(target)
super(URI(target == Paperclip::UrlGenerator.unescape(target) ? escaped : target), options)
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [96/80]

@kikihakiem
Copy link
Author

@ssinghi URI.escape and other methods use RFC2396_Parser internally. I see that in Ruby 3.0 preview that class is not removed, so I think we can use it if we want the exact same behaviour as URI.escape without introducing a new dependency. Please review

@ssinghi
Copy link

ssinghi commented Oct 11, 2020

Thank you @kikihakiem for this PR, appreciate it.
Thank you @shalinibhawsingka-kreeti and @Kevinrob for the review.

@ssinghi ssinghi merged commit f036773 into kreeti:master Oct 11, 2020
@adrijere
Copy link

Thank you guys for your hard work! Is it possible to do a release with a new tag with the fix please @ssinghi :) ?

@kikihakiem kikihakiem deleted the fix-ruby271-warnings branch October 15, 2020 07:12
@xdmx
Copy link

xdmx commented Nov 26, 2020

Definitely a plus one on the above, I'd love to see a new version released to fix all warnings, without having to fetch the code through git. Also considering that Xmas and 3.0 are near 🎅

@ghiculescu
Copy link

for those who missed it, https://github.com/kreeti/kt-paperclip/releases/tag/v6.4.0 is live now

@masterkain
Copy link

I still have

/bundle/vendor/ruby/2.7.0/bundler/gems/kt-paperclip-76b168fbe68d/lib/paperclip/io_adapters/uri_adapter.rb:56: warning: calling URI.open via Kernel#open is deprecated, call URI.open directly or use URI#open

spaghetticode added a commit to spaghetticode/kt-paperclip that referenced this pull request Jan 17, 2022
This commit is part of the backport of kreeti#38 to Paperclip 4.x.
spaghetticode added a commit to spaghetticode/kt-paperclip that referenced this pull request Jan 17, 2022
This commit is the second and last part of the backport of kreeti#38
to Paperclip 4.x.
spaghetticode added a commit to spaghetticode/kt-paperclip that referenced this pull request Jan 18, 2022
This commit is part of the backport of kreeti#38 to Paperclip 4.x.
spaghetticode added a commit to spaghetticode/kt-paperclip that referenced this pull request Jan 18, 2022
This commit is the second and last part of the backport of kreeti#38
to Paperclip 4.x.
ssinghi pushed a commit that referenced this pull request Feb 21, 2022
This commit is part of the backport of #38 to Paperclip 4.x.
ssinghi pushed a commit that referenced this pull request Feb 21, 2022
This commit is the second and last part of the backport of #38
to Paperclip 4.x.
ssinghi pushed a commit that referenced this pull request Feb 21, 2022
This commit is part of the backport of #38 to Paperclip 4.x.
ssinghi pushed a commit that referenced this pull request Feb 21, 2022
This commit is the second and last part of the backport of #38
to Paperclip 4.x.
lauratpa added a commit to reviewshake-app/paperclip that referenced this pull request Jul 20, 2022
remmons added a commit to AdWerx/paperclip that referenced this pull request Sep 26, 2023
Cherry picked: kreeti#38
Original Issue: kreeti#35

URI.escape is obsolete
The URI.escape documentation (https://ruby-doc.org/stdlib-2.7.1/libdoc/uri/rdoc/URI/Escape.html#method-i-escape-label-Description) has this statement:
remmons added a commit to AdWerx/paperclip that referenced this pull request Sep 26, 2023
Cherry picked: kreeti#38
Original Issue: kreeti#35

URI.escape is obsolete
The URI.escape documentation (https://ruby-doc.org/stdlib-2.7.1/libdoc/uri/rdoc/URI/Escape.html#method-i-escape-label-Description) has this statement:

"This method is obsolete and should not be used. Instead, use
CGI.escape, URI.encode_www_form or URI.encode_www_form_component
depending on your specific use case."
remmons added a commit to AdWerx/paperclip that referenced this pull request Sep 26, 2023
Cherry picked: kreeti#38
Original Issue: kreeti#35

URI.escape is obsolete
The URI.escape documentation (https://ruby-doc.org/stdlib-2.7.1/libdoc/uri/rdoc/URI/Escape.html#method-i-escape-label-Description) has this statement:

"This method is obsolete and should not be used. Instead, use
CGI.escape, URI.encode_www_form or URI.encode_www_form_component
depending on your specific use case."
remmons added a commit to AdWerx/paperclip that referenced this pull request Sep 26, 2023
Cherry picked: kreeti#38
Original Issue: kreeti#35

URI.escape is obsolete
The URI.escape documentation (https://ruby-doc.org/stdlib-2.7.1/libdoc/uri/rdoc/URI/Escape.html#method-i-escape-label-Description) has this statement:

"This method is obsolete and should not be used. Instead, use
CGI.escape, URI.encode_www_form or URI.encode_www_form_component
depending on your specific use case."
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.

Fix warning : URI.escape is obsolete
8 participants