Support rack 2 #151

junaruga · 2016-10-21T13:23:22Z

Now rack latest version is 2.0.1
https://rubygems.org/gems/rack

And latest version Rails 5.0.0.1 has used rack 2 with latest version rack-test.
https://github.com/rails/rails/blob/v5.0.0.1/actionpack/actionpack.gemspec#L24

Also sinatra 2.0.0beta has used rack2
https://rubygems.org/gems/sinatra/versions/2.0.0.beta2

So, I think that it's time for our rack-test to support rack 2.x.

Currently we need to re-create Gemfile.lock after this PR.

rm Gemfile.lock
bundle install --path vendor/bundle

However personally I think we do not need to manage Gemfile.lock on github.
If the module is complicated such as Rails, it may be useful to manage Gemfile.lock on github.
https://github.com/rails/rails/blob/v5.0.0.1/Gemfile.lock

But if the module is not complicated such "rack", it may be better not to manage Gemfile.lock on github with set it on .gitignore.
https://github.com/rack/rack/blob/2.0.1/.gitignore#L8

echo -e '\n/Gemfile.lock' >> .gitignore
git rm Gemfile.lock

I included this pull-request's commit hash too.
#148

junaruga · 2016-10-27T19:24:34Z

@brynary Hello, are you there?

perlun · 2017-04-26T05:55:57Z

Also related to #81

@junaruga - I think the right thing is to remove Gemfile.lock as you suggest; for gem authoring, it should be committed to the repo.

junaruga · 2017-04-26T08:00:45Z

@perlun yes I think so. Removing Gemfile.lock is better.
I will remove Gemfile.lock and do rebase to use Travis test in this PR.

junaruga · 2017-04-26T11:52:07Z

@perlun
I removed Gemfile.lock.
I did rebase on HEAD of latest mater branch. and all the test was passed.
Can you merge this?

By the way, below part of the modification in this PR is to prevent test error for bundle exec rake spec.

-gem "codeclimate-test-reporter", group: :test, require: nil
+# Keep version < 1 to supress deprecated warning temporary.
+gem "codeclimate-test-reporter", "< 1", group: :test, require: nil

$ bundle exec rake spec
/usr/local/ruby-2.4.1/bin/ruby -w -I/home/jaruga/git/rack-test/vendor/bundle/ruby/2.4.0/gems/rspec-core-3.5.4/lib:/home/jaruga/git/rack-test/vendor/bundle/ruby/2.4.0/gems/rspec-support-3.5.0/lib /home/jaruga/git/rack-test/vendor/bundle/ruby/2.4.0/gems/rspec-core-3.5.4/exe/rspec --pattern spec/\*\*/\*_spec.rb
W, [2017-04-26T13:20:42.364641 #9368]  WARN -- :       This usage of the Code Climate Test Reporter is now deprecated. Since version
      1.0, we now require you to run `SimpleCov` in your test/spec helper, and then
      run the provided `codeclimate-test-reporter` binary separately to report your
      results to Code Climate.

      More information here: https://github.com/codeclimate/ruby-test-reporter/blob/master/README.md

/usr/local/ruby-2.4.1/bin/ruby -w -I/home/jaruga/git/rack-test/vendor/bundle/ruby/2.4.0/gems/rspec-core-3.5.4/lib:/home/jaruga/git/rack-test/vendor/bundle/ruby/2.4.0/gems/rspec-support-3.5.0/lib /home/jaruga/git/rack-test/vendor/bundle/ruby/2.4.0/gems/rspec-core-3.5.4/exe/rspec --pattern spec/\*\*/\*_spec.rb failed

perlun · 2017-04-26T11:57:12Z

Gemfile

@@ -2,7 +2,8 @@ source 'https://rubygems.org'

 gem 'rspec'
 gem "rack"
-gem "sinatra"
+gem "sinatra", "~> 2.0.0.rc2"


Does this mean we now only support Rack 2 or does the code still work w/ Rack 1? Can we be sure? 😄 I think it would be good if the gem properly supports both Rack 1 and Rack 2 for the time being, especially since Sinatra 2.x hasn't reached version 2.0.0 yet.

Maybe something like Appraisal could be used

@perlun sure, I will agree. thanks for the mentioning. I think better solution is to test both Rack 1 and Rack 2 on Travis. I will change files for that. Just moment.

@dentarg maybe I will prepare another gemfile such as gemfiles/Gemfile.rack1, or write if condition to Gemfile.

perlun · 2017-04-26T12:47:28Z

@junaruga - see my question above.

junaruga · 2017-04-26T13:56:22Z

@perlun ok I will change the files.

junaruga · 2017-04-26T15:34:54Z

@perlun

I prepared both rack 2 and 1 test to use 2 Gemfiles referring below gem packages' cases.
https://github.com/rails/sprockets-rails/blob/master/.travis.yml
https://github.com/svenfuchs/i18n/blob/master/.travis.yml

Ideally, I think common gems for rack 2 and rack 1 test should be managed in rack-test.gemspec file. not for each Gemefile.

As rack-test.gemspec is generated from Thorfile by thor command (bundle exec thor :gemspec), the gems should be managed in Thorfile.

However I do not want to update rack-test.gemspec in this PR.
I want to update it in another PR.

I want to update Thorfile for another improvement too.
For example, I want to remove test and development specified files from rack-test gem file.

As a information, use cases for thor is

$ bundle install --path vendor/bundle
$ bundle exec thor help

junaruga · 2017-04-26T18:12:14Z

@perlun ah just moment. I want to rebase again.

junaruga · 2017-04-26T18:23:31Z

@perlun ok I did rebase again.
There are 2 commits.
Original 1 commit to support rack 2, and my additional commit.

I think that this style's merit is, after "Squash and merge", I guess that we can see 2 commits' information from github web UI.
I want to keep commit for original author to respect original author as much as possible.

junaruga · 2017-04-27T11:12:47Z

@perlun I know that basically we only accept "Squash and merge".

But in this case, when I consider about this today again, I want to commit these 2 commits keeping original commit to respect original contributor.
I think respecting each contributor is more important than removing merge commit, keeping easy understanding commit.

perlun · 2017-04-28T20:19:36Z

@junaruga Fair enough. I changed the options to support "rebase merge":

It means both commits will be retained, so you preserve the original author commit unchanged, and your commit on top of it. Is that acceptable from your POV? If not, just let me know.

junaruga · 2017-04-28T21:21:32Z

@perlun yes I can accept it. Great. Thanks. Can you approve the review for the PR?

perlun

One idea that I'm thinking about. Will it work? If not, let me know and I'll approve it as-is.

perlun · 2017-04-29T18:31:43Z

.travis.yml

 rvm:
  - 2.2.7
  - 2.3.4
  - 2.4.1
  - ruby-head
  - jruby-9.1.8.0
  - jruby-head
+gemfile:
+  - Gemfile
+  - gemfiles/Gemfile.rack-1


Would it be possible to move this file to the top-level? I.e. to name it Gemfile.rack-1.x or something in the top-level folder instead. Makes it easier to find it that way.

@perlun ok, it looks good. As we still have only 2 kind of Gemfile, moving it to the top-level makes sense. Let me try it.

junaruga · 2017-05-01T12:26:56Z

One idea that I'm thinking about. Will it work? If not, let me know and I'll approve it as-is.

It makes sense. After changing on your way, it works!

perlun · 2017-05-02T07:58:49Z

spec/rack/test/utils_spec.rb

@@ -66,7 +66,7 @@
        :input => StringIO.new(data)
      }
      env = Rack::MockRequest.env_for("/", options)
-      params = Rack::Utils::Multipart.parse_multipart(env)
+      params = Rack::Multipart.parse_multipart(env)


@junaruga Does the Rack::Multipart class/module exist in Rack 1.x as well? I mean, is this approach fine with both Rack versions?

@perlun yes it exists in Rack 1.x too. Yes it is fine for both rack 1.x and 2.x. Because Rack::Multipart exists in both versions.

This modification is because rack project have just deleted the code to access from Rack::Utils to Multipart from rack 2.x.

Below are the commit in rack. and You can also see this kind of modification (replacement) in the commit.

rack/rack@b2d7396

$ git show b2d73960e9ea6b8b15321ef190f13a290d1aedf0 lib/rack/utils.rb ... - Multipart = Rack::Multipart - ...

OK, great. Then I will approve and merge this now. Thanks @junaruga!

perlun

Approved now. I will let you merge so you get the rebasing done right.

* Prepare test environment for both rack 1 and rack 2 on Travis. * Update .travis.yml. * Add "bundle list" in script section to check rack's version from Travis log page. * Update Gemfile for rack 2 test, and add gemfiles/Gemfile.rack-1 for rack 1 test. * Keep codeclimate-test-reporter version < 1. To supress deprecated warning temporary. Later update it to latest version. * Add gems to Gemfile for Thorfile. * Specify sinatra 2.0.0.rc2 version for rack 2 test. * Delete Gemfile.lock * Fix test suite for Rack 2.x compatiblity.

junaruga · 2017-05-02T15:03:10Z

@perlun Thanks! Merged!

perlun · 2017-05-02T18:54:08Z

@junaruga I was reminded that the "rebase individual commits" option is a bit ugly in that it does not keep any track whatsoever of the PR number (#151) and the new, rebased commits. This is not so nice I think, so maybe it's better to use the regular merge commits in these cases (even though I still think "squashed merge" is better for many cases). I'll change the settings accordingly.

junaruga · 2017-05-03T08:20:19Z

@perlun OK.
I am sorry that one of those issues may be my fault. I had seen my 2 commits and your additional 2 commits (merge commits). And as I remembered you said that you did not like a merge commit, I thought your 2 merge commits were something like mistake. And I removed the 2 merge commits by myself.
Yes, I could not write additional comment such as (#151) on the option. It is a little bit inconvenient.

perlun · 2017-05-03T09:55:49Z

I am sorry that one of those issues may be my fault.

It's no big deal Jun. I think the problem is with how GitHub handles "rebase individual commits". Take a look at 3e6fc7c I was about to say, but hey... it does actually show now in the GitHub UI that it comes from #151 after all. That's nice! I'll reenable the "rebase individual commits" option after all, it isn't that bad as I thought. 😆

junaruga · 2017-05-03T10:39:26Z

Oh okay. I am seeing the commit page. Good catch, Github UI!

junaruga mentioned this pull request Apr 26, 2017

About maintaining #135

Closed

junaruga force-pushed the feature/rack-2 branch from 4f5dfb7 to a3d78bf Compare April 26, 2017 11:43

perlun reviewed Apr 26, 2017

View reviewed changes

junaruga force-pushed the feature/rack-2 branch 2 times, most recently from a39dead to 7a9148a Compare April 26, 2017 15:29

junaruga force-pushed the feature/rack-2 branch from 7a9148a to 211bbeb Compare April 26, 2017 18:18

perlun reviewed Apr 29, 2017

View reviewed changes

junaruga force-pushed the feature/rack-2 branch from 211bbeb to d308d79 Compare May 1, 2017 12:20

perlun reviewed May 2, 2017

View reviewed changes

perlun approved these changes May 2, 2017

View reviewed changes

voxik and others added 2 commits May 2, 2017 16:45

Fix test suite for Rack 2.x compatiblity.

35cc93b

junaruga force-pushed the feature/rack-2 branch from 9b7a960 to 31ec332 Compare May 2, 2017 14:46

junaruga merged commit 3a69fc2 into rack:master May 2, 2017

junaruga deleted the feature/rack-2 branch May 2, 2017 15:02

junaruga mentioned this pull request May 3, 2017

Fix test suite for Rack 2.x compatiblity. #148

Closed

junaruga mentioned this pull request May 15, 2017

Updates README.rdoc with code example #172

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support rack 2 #151

Support rack 2 #151

junaruga commented Oct 21, 2016 •

edited

Loading

junaruga commented Oct 27, 2016

perlun commented Apr 26, 2017

junaruga commented Apr 26, 2017

junaruga commented Apr 26, 2017

perlun Apr 26, 2017 •

edited

Loading

dentarg Apr 26, 2017

junaruga Apr 26, 2017

perlun commented Apr 26, 2017

junaruga commented Apr 26, 2017

junaruga commented Apr 26, 2017 •

edited

Loading

junaruga commented Apr 26, 2017

junaruga commented Apr 26, 2017

junaruga commented Apr 27, 2017

perlun commented Apr 28, 2017

junaruga commented Apr 28, 2017

perlun left a comment

perlun Apr 29, 2017

junaruga May 1, 2017

junaruga commented May 1, 2017 •

edited

Loading

perlun May 2, 2017 •

edited

Loading

junaruga May 2, 2017 •

edited

Loading

perlun May 2, 2017

perlun left a comment

junaruga commented May 2, 2017

perlun commented May 2, 2017

junaruga commented May 3, 2017

perlun commented May 3, 2017

junaruga commented May 3, 2017

Support rack 2 #151

Support rack 2 #151

Conversation

junaruga commented Oct 21, 2016 • edited Loading

junaruga commented Oct 27, 2016

perlun commented Apr 26, 2017

junaruga commented Apr 26, 2017

junaruga commented Apr 26, 2017

perlun Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

dentarg Apr 26, 2017

Choose a reason for hiding this comment

junaruga Apr 26, 2017

Choose a reason for hiding this comment

perlun commented Apr 26, 2017

junaruga commented Apr 26, 2017

junaruga commented Apr 26, 2017 • edited Loading

junaruga commented Apr 26, 2017

junaruga commented Apr 26, 2017

junaruga commented Apr 27, 2017

perlun commented Apr 28, 2017

junaruga commented Apr 28, 2017

perlun left a comment

Choose a reason for hiding this comment

perlun Apr 29, 2017

Choose a reason for hiding this comment

junaruga May 1, 2017

Choose a reason for hiding this comment

junaruga commented May 1, 2017 • edited Loading

perlun May 2, 2017 • edited Loading

Choose a reason for hiding this comment

junaruga May 2, 2017 • edited Loading

Choose a reason for hiding this comment

perlun May 2, 2017

Choose a reason for hiding this comment

perlun left a comment

Choose a reason for hiding this comment

junaruga commented May 2, 2017

perlun commented May 2, 2017

junaruga commented May 3, 2017

perlun commented May 3, 2017

junaruga commented May 3, 2017

junaruga commented Oct 21, 2016 •

edited

Loading

perlun Apr 26, 2017 •

edited

Loading

junaruga commented Apr 26, 2017 •

edited

Loading

junaruga commented May 1, 2017 •

edited

Loading

perlun May 2, 2017 •

edited

Loading

junaruga May 2, 2017 •

edited

Loading