Fix many ruby warnings

[cllns]

Addresses most of #44.

Fixing warning: URI.escape is obsolete and warning: URI.unescape is obsolete required adding a dependency, on Addressable. I hope that's OK!

All the tests pass (except for examples/static/config.ru doesn't run for me, regardless of these changes, see: #46).

Note: this doesn't fix all the Ruby warnings in this project, but rather the ones that hanami-router hits in its test suite. I tried to fix others, but they weren't easy fixes (instance_eval redefining a method). To run the tests with warnings enabled, you can do: RUBYOPT=w bundle exec rake test

Also, I couldn't fix one of the warnings that hanami-router hits: lib/http_router/route_helper.rb:99: warning: assigned but unused variable - params, because the local variable is referenced in the eval call a couple lines down, in one of the tests :/

[AlfonsoUceda]

Hi @joshbuddy can you take a look at this PR? thanks so much ;)

[weppos]

I've just landed here, trying to understand the reason of the hanami warnings.

I also read http://stackoverflow.com/a/34275638/123527, and specifically

As I said above, current URI.encode is wrong on spec level. So we won't provide the exact replacement. The replacement will vary by its use case.

This is extremely sad, and confusing!

[weppos]

@cllns @AlfonsoUceda FYI there is also another solution, that doesn't require to bring in the Addressable dependency.

Simply replace:

class HttpRouter
  class Request
    attr_accessor :path, :params, :rack_request, :extra_env, :continue, :passed_with, :called
    attr_reader :acceptable_methods
    alias_method :rack, :rack_request
    alias_method :called?, :called

    def initialize(path, rack_request)
      @rack_request = rack_request
      @path = URI.unescape(path).split(/\//)

to

class HttpRouter
  class Request
    attr_accessor :path, :params, :rack_request, :extra_env, :continue, :passed_with, :called
    attr_reader :acceptable_methods
    alias_method :rack, :rack_request
    alias_method :called?, :called

    def initialize(path, rack_request)
      @rack_request = rack_request
      @path = URI::DEFAULT_PARSER.unescape(path).split(/\//)

The relevant line is

      @path = URI::DEFAULT_PARSER.unescape(path).split(/\//)

This is the current implementation of URI#unescape

    def unescape(*arg)
      warn "#{caller(1)[0]}: warning: URI.unescape is obsolete" if $VERBOSE
      DEFAULT_PARSER.unescape(*arg)
    end

Why I think this is a reasonable (perhaps not the best, but reasonable) solution:

• it doesn't add a dependency
• it relies on a constant which is public and it is supposed to be supported by Ruby and treated as public API.
• Ruby maintainers are notably very careful about keeping BC (sometimes even too much), which makes me think that constant will be there for a long time
• The method is obsolete since 2009. I don't expect it to change anytime soon. And if they should remove it, at that point the entire URI library (including the constant) would likely have to be removed, making the change definitely only for a major release change (e.g. Ruby 3).

[AlfonsoUceda]

@weppos thanks for bringing up that solution I think it's good for us and we could create a PR in hanami/controller to overwrite the initializer, what do you think?

[weppos]

I'm not very familiar with how hanami/controller deals with the router. I would not recommend to override an entire initializer for the long term solution, but given this library seems to be quite static, it may be a temporary solution.

/cc @jodosha

[PhilT]

Seeing the http_router warnings in test output in a Hanami app. Anything I can do to help get this one in?