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

Drop thin support #1627

Merged
merged 7 commits into from Aug 8, 2020
Merged

Conversation

rkmathi
Copy link
Contributor

@rkmathi rkmathi commented Jul 26, 2020

fixes #1624

Remove thin to drop support.
Use puma or rainbows instead of thin.

@rkmathi rkmathi mentioned this pull request Jul 26, 2020
Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

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

Hey Ryuichi, thanks for working on this! Mostly looks good, but please review a few points I commented.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
lib/sinatra/base.rb Show resolved Hide resolved
test/integration_test.rb Show resolved Hide resolved
Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

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

Hey Ryuichi,
Thanks for continuing your efforts. Please take a look at my comments.

@rkmathi rkmathi force-pushed the rkmathi/drop-thin-support branch 5 times, most recently from a032ccb to 96d1f0b Compare August 5, 2020 02:30
@rkmathi rkmathi requested a review from namusyaka August 5, 2020 10:56
Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

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

LGTM with a few nits. Thanks so much for your hard working on the complex issue!

@@ -3,7 +3,6 @@
# run *one* of these:
#
# rackup -s mongrel stream.ru # gem install mongrel
# thin -R stream.ru start # gem install thin
Copy link
Member

Choose a reason for hiding this comment

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

Please copy test/integration/rainbows.conf here and add rainbows -c rainbows.conf stream.ru.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 7a96472

@@ -1,6 +1,10 @@
$stderr.puts "loading"
require 'sinatra'

if RUBY_ENGINE == "ruby"
require_relative './rainbows'
end
Copy link
Member

Choose a reason for hiding this comment

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

require_relative 'rainbows' if RUBY_ENGINE == 'ruby'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rkmathi
Copy link
Contributor Author

rkmathi commented Aug 8, 2020

@namusyaka I fixed.
Thank you for reviewing and helping me a lot!

@rkmathi rkmathi requested a review from namusyaka August 8, 2020 10:23
Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

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

Awesome!

@namusyaka namusyaka merged commit f57acef into sinatra:master Aug 8, 2020
rkmathi added a commit to rkmathi/sinatra that referenced this pull request Aug 8, 2020
@jkowens
Copy link
Member

jkowens commented Aug 8, 2020

Great work @rkmathi! 💯 👏

@namusyaka namusyaka added this to the v2.1.0 milestone Aug 30, 2020
jkowens added a commit that referenced this pull request Oct 1, 2020
namusyaka added a commit that referenced this pull request Nov 5, 2020
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.

drop thin support
3 participants