Fix sample app activerecord #110
Conversation
This allows the tests to run without failing
Add an explicit `rake db:migrate` step for `RAILS_ENV=test` This allows the test to run
Generated by 🚫 danger |
Replace the `* Your contribution here.` and add the PR link now I know it!
CHANGELOG.md
Outdated
| @@ -3,6 +3,7 @@ | |||
| #### 0.11.2 (Next) | |||
|
|
|||
| * Your contribution here. | |||
| * [#110](https://github.com/slack-ruby/slack-ruby-bot-server/pull/110): Update the sample_app_activerecord Gemfile and README to make them function as expected. | |||
There was a problem hiding this comment.
Needs a format fix (include your name).
I would also just write "Fix ActiveRecord sample app", short and concise
|
Build is failing on rubocop, run |
🤦 Yup, just had that on my clone of this... gem 'virtus'
gem 'activerecord', '~> 5.0.0', require: 'active_record'
gem 'newrelic-slack-ruby-bot'
<snip>works, the gap between virtus and activerecord is enough for rubocop. # Virtus needs to be loaded before activerecord
gem 'virtus'
gem 'activerecord', '~> 5.0.0', require: 'active_record'
gem 'newrelic-slack-ruby-bot'
<snip>? |
Virtus needs to be loaded before activerecord but rubocop alerts when sequential gems are not in alphabetical order
|
Tests against AR are still failing. LMK if you need help. Btw, while adding virtus seems to fix problems, I wonder why we need it at all. Grape removed this dependency in 1.3.0, see https://github.com/ruby-grape/grape/blob/master/UPGRADING.md#upgrading-to--130. |
Not just the sampleapp version. The reference to Virtus being needed seemd to come from ActiveRecord rather than Grape
|
Merged, thanks. |
|
Can we figure out why we need Virtus at all though with the latest version(s) of Grape? |
|
This is actually quite simple. Virtus used to have a Boolean definition and Grape used it by default. Then Grape implemented its own, and got rid of the Virtus dependency. I updated the boolean definition in #111 instead of taking the virtus dependency. |
This is designed to address some of the issues raised in #109