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 detect_rack_handler #1652

Merged
merged 1 commit into from Oct 14, 2020
Merged

remove detect_rack_handler #1652

merged 1 commit into from Oct 14, 2020

Conversation

ceclinux
Copy link
Contributor

@ceclinux ceclinux commented Oct 8, 2020

detect_rack_handler can be remove becaue it has already been implemented in Rack::Handler.pick

@jkowens
Copy link
Member

jkowens commented Oct 8, 2020

Nice!

@namusyaka
Copy link
Member

I think that merging this would require additional work (e.g. document changes about supporting servers) probably, but I haven't specifically identified them.

@jkowens
Copy link
Member

jkowens commented Oct 11, 2020

@namusyaka it looks like Rack::Handler.pick is functionally identical except for the error message:

    # Select first available Rack handler given an `Array` of server names.
    # Raises `LoadError` if no handler was found.
    #
    #   > pick ['puma', 'webrick']
    #   => Rack::Handler::WEBrick
    def self.pick(server_names)
      server_names = Array(server_names)
      server_names.each do |server_name|
        begin
          return get(server_name.to_s)
        rescue LoadError, NameError
        end
      end

      raise LoadError, "Couldn't find handler for: #{server_names.join(', ')}."
    end

@namusyaka
Copy link
Member

Ah I seemed to misunderstand meaning of the server, that fully makes sense.

@namusyaka namusyaka added this to the v2.1.1 milestone Oct 11, 2020
@namusyaka
Copy link
Member

Btw, do we need to lock the version of rack?
I'm not sure when the method was introduced.

@jkowens
Copy link
Member

jkowens commented Oct 11, 2020

It's been there since Rack 1.5: rack/rack@77506e4

@namusyaka
Copy link
Member

cool, i have zero concern now.

@jkowens jkowens merged commit 578c1a6 into sinatra:master Oct 14, 2020
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.

None yet

3 participants