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

Remove call to deprecated URI.escape #188

Merged
merged 1 commit into from
Jun 28, 2020
Merged

Conversation

dmagliola
Copy link
Collaborator

This method has been deprecated for a while, and it gives us warnings in Ruby 2.7

This method has been deprecated for a while, and it gives us warnings in
Ruby 2.7

Signed-off-by: Daniel Magliola <danielmagliola@gocardless.com>
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f31bdcb on remove_deprecated_escaping into c80c550 on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f31bdcb on remove_deprecated_escaping into c80c550 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f31bdcb on remove_deprecated_escaping into c80c550 on master.

Copy link
Member

@Sinjo Sinjo left a comment

Choose a reason for hiding this comment

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

So after our call I went away and did some reading. This post was one of the clearer explanations I found.

It seems that it's hard to find a perfect answer in stdlib, including the one we were already using in URI.escape. There's one in the addressable gem, but I'm loath to drag that in just for this push gateway code when it's not clear that the differences in approach make any practical difference for thus usage.

This change seems like a strict upgrade, so let's go with it.

@dmagliola dmagliola merged commit efad822 into master Jun 28, 2020
@dmagliola dmagliola deleted the remove_deprecated_escaping branch June 28, 2020 18:28
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.

3 participants