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 obsolete uri escape warning #58

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

emilford
Copy link

Prior to these changes, URI.escape would trigger a warning. Instead, this uses Addressable::URI.escape to maintain similar behavior, but without the warning. One difference between the two being how the # sign is handled. URI.escape escapes this character. Addressable::URI.escape does not.

See this link for more information:

https://stackoverflow.com/a/13059657

Resolves: #45

The `--debugger` option in `.rspec` was misspelled and is also no longer
a valid option. The gem also does not currently have a
`spec/spec_helper.rb` file.

Co-authored-by: Daniel Colson <[email protected]>
@emilford emilford force-pushed the fix-obsolete-uri-escape-warning branch from 3240ba4 to e3c71d1 Compare July 30, 2020 21:55
Prior to these changes, `URI.escape` would trigger a warning. Instead,
this uses `Addressable::URI.escape` to maintain similar behavior, but
without the warning. One difference between the two being how the `#`
sign is handled. `URI.escape` escapes this character.
`Addressable::URI.escape` does not.

See this link for more information:

https://stackoverflow.com/a/13059657

Co-authored-by: Daniel Colson <[email protected]>
@emilford emilford force-pushed the fix-obsolete-uri-escape-warning branch from e3c71d1 to f433e82 Compare July 30, 2020 21:56
@emilford
Copy link
Author

The existing tests also fail on master: https://travis-ci.org/github/docusign/docusign-ruby-client/branches.

@emilford
Copy link
Author

emilford commented Aug 5, 2020

👋 @LarryKlugerDS, any thoughts on this PR? Thanks!

@nicolasrouanne
Copy link

Could we have this merged please? 🙏
Docusign is the only gem in my projects raising warnings in ruby 2.7, it's really annoying 😢

This is what my tests look like now
Screen Shot 2020-09-02 at 10 35 22

@LarryKlugerDS
Copy link
Contributor

I'm sorry for the slow response. I have opened DCM-4886 for engineering to review the PR

@matheusbsilva
Copy link

Any updates on this?

@MatheusRich
Copy link

Instead of adding another gem, is it possible just to change the call to URI.encode_www_form_component?

@sposin
Copy link

sposin commented Feb 8, 2021

@LarryKlugerDS Any updates on this?

@joshkinabrew
Copy link

URI.escape is completely removed in Ruby 3. This needs to be merged before full Ruby 3 support.

I don't think this is the correct way of fixing it (adding another gem) - I agree with @MatheusRich 's solution of just using URI.encode_www_form_component as it seems like the logical replacement.

I would love to get this fix out ASAP @LarryKlugerDS so I don't have to run on a fork.

@barash-asenov
Copy link

barash-asenov commented Jun 2, 2021

@joshkinabrew This is what happens when you use URI.encode_www_form_component on a full URL

URI.encode_www_form_component('https://www.google.com')

will yield the result of
https%3A%2F%2Fwww.google.com

Addressable::URI.encode('https://www.google.com')

will yield
https://www.google.com

URI.encode_www_form_componenet is quite similar to CGI escape and I think meant to be used on only query params rather than the full URL
https://stackoverflow.com/questions/2824126/whats-the-difference-between-uri-escape-and-cgi-escape

Also the encoding style is different

URI.encode_www_form_component('space space')

yields

space+space

and

Addressable::URI.encode('space space')

yields

space%20space as it was with URI.encode as I remember.

I agree that it is quite bad to bring extra dependency but I also think this is the fastest fix. Otherwise, it will be quite a big work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URI.escape obsolete warning on Ruby 2.7+
8 participants