Drops action_filter support #276

bgreg · 2017-07-17T18:31:17Z

This small change fixes the rails 5.1.2 error, and hard codes the after_action callback.

tute · 2017-07-17T18:43:51Z

Yep! Shall we update the Travis CI matrix to disallow unsupported Rails' versions, and do a major release? Thanks!

tute · 2017-07-17T19:25:52Z

Related with: #274. We'll also want to update the gemspec.

bgreg · 2017-07-17T19:34:09Z

I'm looking at that travis build now.

tute · 2017-07-17T19:47:50Z

I'm fine ignoring most Hound comments in this PR. Thanks for your work!

bgreg · 2017-07-17T19:49:25Z

@tute What do you think about this case statement from the Gemfile. Looks like it will be dead code too:

# /Gemfile
version = ENV['RAILS_VERSION'] || '5.0'
rails = case version
when 'master'
  { github: 'rails/rails' }
when '4.0-protected-attributes'
  gem 'protected_attributes'
  "~> 4.0.0"
when /4\.0|4\.1/
  "~> #{version}.0"
when /4\.2/
  "~> #{version}.0.rc3"
when '3.2'
  gem 'strong_parameters'
  "~> #{version}.0"
end

tute · 2017-07-17T19:52:48Z

@tute What do you think about this case statement from the Gemfile. Looks like it will be dead code too:

Yep! We can delete the rails local variable, and use version instead. Love this.

bgreg · 2017-07-17T19:54:22Z

Cool, will do. Also, there is something I don't understand about travis. Why is it running versions of rails other than what I have specified on this branch? Is it also using the master version of the .travis.yml file?

tute · 2017-07-17T19:57:48Z

Cool, will do. Also, there is something I don't understand about travis. Why is it running versions of rails other than what I have specified on this branch? Is it also using the master version of the .travis.yml file?

It looks like! Shouldn't be running 4.2. Let's ignore those for now.

tute · 2017-07-17T20:47:17Z

Don't forget it seems we can clean up some code in test/dummy/config/environments/test.rb.

tute · 2017-07-17T20:47:38Z

I have no problem with this, given people on 5 can use merit ~2. But curious, what bugs were we hitting?

I'd rather keep 5 if we can. 5.0 and 5.1 are similar enough for us.

bgreg · 2017-07-17T21:00:16Z

I was getting 'ArgumentError: key must be 32 byte' when running against 5.0.0.0. Oddly enough I can't reproduce that again. I'll give travis another chance at ~> 5.0, and fix that test.rb file.

bgreg · 2017-07-17T21:20:34Z

Dropping that from the dummy env files causes:

config.eager_load is set to nil. Please update your config/environments/*.rb files accordingly:

  * development - set it to false
  * test - set it to false (unless you use a tool that preloads your test environment)
  * production - set it to true

But turning off warnings seems nice. Sending another build

bgreg · 2017-07-17T21:46:55Z

I'm stuck on this failure right now: https://travis-ci.org/merit-gem/merit/jobs/254626719

bgreg · 2017-07-17T21:54:20Z

@tute could use some input, running out of easy fix ideas for this:

Finished in 1.759557s, 25.5746 runs/s, 74.4506 assertions/s.
/home/travis/build/merit-gem/merit/vendor/bundle/ruby/2.4.0/gems/railties-5.1.0/lib/rails/test_unit/minitest_plugin.rb:9:in `aggregated_results': wrong number of arguments (given 1, expected 0) (ArgumentError)
	from /home/travis/build/merit-gem/merit/vendor/bundle/ruby/2.4.0/gems/minitest-5.10.2/lib/minitest.rb:597:in `report'
	from /home/travis/build/merit-gem/merit/vendor/bundle/ruby/2.4.0/gems/minitest-5.10.2/lib/minitest.rb:687:in `each'
	from /home/travis/build/merit-gem/merit/vendor/bundle/ruby/2.4.0/gems/minitest-5.10.2/lib/minitest.rb:687:in `report'
	from /home/travis/build/merit-gem/merit/vendor/bundle/ruby/2.4.0/gems/minitest-5.10.2/lib/minitest.rb:141:in `run'
	from /home/travis/build/merit-gem/merit/vendor/bundle/ruby/2.4.0/gems/railties-5.1.0/lib/rails/test_unit/minitest_plugin.rb:77:in `run'
	from /home/travis/build/merit-gem/merit/vendor/bundle/ruby/2.4.0/gems/minitest-5.10.2/lib/minitest.rb:63:in `block in autorun'
rake aborted!

What do you think?

tute · 2017-07-17T22:01:45Z

Apparently minitest 5.10.2 has an issue. We can specify versions minitest '~> 5.10', '!= 5.10.2', or later.

bgreg · 2017-07-17T22:19:45Z

Making progress. Just some trial and error finding the lowest rails 5 version to support.

bgreg · 2017-07-18T17:03:16Z

@tute if you agree, can you click resolve on that travis file?

tute · 2017-07-18T17:46:26Z

aha, that means your branch was born before latest master.

Can you please squash your commits together, and rebase them on top of latest master? I wrote instructions for this in https://robots.thoughtbot.com/git-interactive-rebase-squash-amend-rewriting-history. That'd resolve the conflicts in the process. Let me know if I can help you do this.

Thanks for all the work! :)

tute · 2017-07-18T17:47:58Z

Rakefile

-  t.pattern = 'test/**/*_test.rb'
-  t.verbose = true
+  t.pattern  = 'test/**/*_test.rb'
+  t.verbose  = true


No need of the extra white space here

tute · 2017-07-18T17:48:04Z

lib/merit/controller_extensions.rb

-      else
-        base.after_filter :log_and_process
-      end
+      base.after_action :log_and_process


tute · 2017-07-18T17:48:21Z

.travis.yml


 env:
-  - RAILS_VERSION=5.0
+  - RAILS_VERSION=5.0.3
+  - RAILS_VERSION=5.1


There will be changes to merge from latest master, when you rebase you'll see them.

This change drops some version specific code as well as moving the dummy test app to be rails 5 compatible.

houndci-bot · 2017-07-18T19:10:42Z

test/dummy/db/migrate/20140906225844_create_players.rb

@@ -1,4 +1,4 @@
-class CreatePlayers < ActiveRecord::Migration
+class CreatePlayers < ActiveRecord::Migration[5.0]
