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

Add addressable library to Gemfile #17502

Closed
wants to merge 2 commits into from
Closed

Add addressable library to Gemfile #17502

wants to merge 2 commits into from

Conversation

djberg96
Copy link
Contributor

This PR adds the addressable gem to the Gemfile.

From the github project page: Addressable is a replacement for the URI implementation that is part of Ruby's standard library. It more closely conforms to RFC 3986, RFC 3987, and RFC 6570 (level 4), additionally providing support for IRIs and URI templates.

We already get warnings from rubocop regarding URI.escape. Let's just use a better library.

@bdunne
Copy link
Member

bdunne commented Jun 12, 2018

@djberg96 Does this fix the username / password encoding issues for rest-client?

@djberg96
Copy link
Contributor Author

@bdunne If you can be more specific I can look into it, but I'm not immediately sure what you're referring to.

@bdunne
Copy link
Member

bdunne commented Jun 12, 2018

@djberg96
Copy link
Contributor Author

@bdunne I don't think so since that would require internal modifications of the rest-client library.

@bdunne
Copy link
Member

bdunne commented Jun 12, 2018

Does it fix anything other than the rubocop warning?

@djberg96
Copy link
Contributor Author

Generally speaking, it handles special characters better. It also supports url templates, which might be useful in testing.

Some relevant links:

http://rawsyntax.com/blog/url-validation-in-rails-3-and-ruby-in-general
http://cloudspace.com/blog/2009/05/26/replacing-rubys-uri-with-addressable#.WyAaWDNKjf8

@miq-bot
Copy link
Member

miq-bot commented Nov 6, 2018

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Nov 12, 2018

Checked commits https://github.com/djberg96/manageiq/compare/ea97113068ae74ae4188917bc6bdde8382828fcf~...30d4b7e957178cb367363403912fe72a2afca5bc with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@jrafanie
Copy link
Member

Even though addressable is probably a better library than URI in standard library, I don't think it makes sense to change for the sake of change. If there are clear bugs and performance problems with URI that addressable addresses 🤣 , then it makes sense to revisit this. I'm not sure what benefit various RFC standards buys us.

My problem is we have lots of our code and gem code that use the URI library and I'm not sure how to have confidence that this code would work or be improved with addressable.

Finally, no code is actually going to be using addressable after this change so it's just unused code.

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

Successfully merging this pull request may close these issues.

4 participants