Navigation Menu

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

Replace deprecated method usage #117

Merged
merged 2 commits into from Jul 23, 2020

Conversation

PikachuEXE
Copy link
Contributor

@PikachuEXE PikachuEXE commented Jun 18, 2020

URI.escape is obsoleted: ruby/ruby@238b979

@PikachuEXE PikachuEXE force-pushed the replace-deprecated-method-usage branch 2 times, most recently from aa8ce17 to bfc769c Compare June 18, 2020 07:55
@PikachuEXE
Copy link
Contributor Author

Will be ready for review once #116 merged, since this is based on that PR
I want to rebase before marking this PR as ready anyway

@PikachuEXE PikachuEXE force-pushed the replace-deprecated-method-usage branch from bfc769c to cda6693 Compare June 18, 2020 08:29
@PikachuEXE PikachuEXE marked this pull request as ready for review June 18, 2020 08:42
@PikachuEXE
Copy link
Contributor Author

Screw it, I rebased on master without changes in #116 :D

@PikachuEXE
Copy link
Contributor Author

PikachuEXE commented Jun 19, 2020

I am not sure if we should use URI::Parser#escape instead which is available in 1.9
https://ruby-doc.org/stdlib-1.9.1/libdoc/uri/rdoc/URI/Parser.html
Sinatra seems using it
sinatra/sinatra-contrib#127

Edit: A test case failed if we use URI::Parser#escape`

fastimage/test/test.rb

Lines 291 to 295 in 8386e11

def test_should_handle_permanent_redirect_with_complex_relative_url
register_redirect(TestUrl, "/pho to.gne?rb=1&short=Vv4Und")
register_redirect("#{TestUrl}pho%20to.gne?rb=1&short=Vv4Und", "/" + GoodFixtures.keys.first)
assert_equal GoodFixtures[GoodFixtures.keys.first][1], FastImage.size(TestUrl, :raise_on_failure=>true)
end

@PikachuEXE PikachuEXE force-pushed the replace-deprecated-method-usage branch from cda6693 to 5d4c06b Compare July 2, 2020 07:01
@PikachuEXE
Copy link
Contributor Author

Rebased on latest master and test passed

@sdsykes
Copy link
Owner

sdsykes commented Jul 2, 2020

Thanks I'll look at this, and aim to make a release shortly.

@PikachuEXE
Copy link
Contributor Author

Reminder to look at this PR~

@PikachuEXE
Copy link
Contributor Author

On vacation?

@sdsykes
Copy link
Owner

sdsykes commented Jul 18, 2020

For another week!

@PikachuEXE
Copy link
Contributor Author

Have a good one!

@sdsykes
Copy link
Owner

sdsykes commented Jul 23, 2020

These are not the same?

irb(main):003:0> ERB::Util.url_encode("/pho to.gne?rb=1&short=Vv4Und")
=> "%2Fpho%20to.gne%3Frb%3D1%26short%3DVv4Und"
irb(main):004:0> URI.escape("/pho to.gne?rb=1&short=Vv4Und")
=> "/pho%20to.gne?rb=1&short=Vv4Und"

@PikachuEXE PikachuEXE force-pushed the replace-deprecated-method-usage branch from 5d4c06b to 0d4dd4a Compare July 23, 2020 07:22
@PikachuEXE
Copy link
Contributor Author

Forgot to test with erb required
Anyway updated to use URI::DEFAULT_PARSER as there is already a patch for old rubies

@PikachuEXE
Copy link
Contributor Author

Build passed
image

Any chance we can see the CI check status in future PR without manually checking it?

@sdsykes sdsykes merged commit c57b07d into sdsykes:master Jul 23, 2020
@PikachuEXE PikachuEXE deleted the replace-deprecated-method-usage branch July 23, 2020 08:38
@PikachuEXE
Copy link
Contributor Author

Hope to see a new release soon~

@PikachuEXE
Copy link
Contributor Author

Will there be a new release this or next week?

@sdsykes
Copy link
Owner

sdsykes commented Jul 29, 2020

I released it already....

@PikachuEXE
Copy link
Contributor Author

Sorry
Forgot watch for new releases
And my gemfile is pointing to specific commit

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