Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
robin850
left a comment
robin850
left a comment
There was a problem hiding this comment.
This looks good, thanks for the pull request !
| add_index :<%= session_table_name %>, :updated_at | ||
| end | ||
| end | ||
| end No newline at end of file |
There was a problem hiding this comment.
Could leave the very last blank line in the file please ? This is intentional.
| end | ||
|
|
||
| def migration_version | ||
| "[#{ActiveRecord::VERSION::MAJOR}.#{ActiveRecord::VERSION::MINOR}]" if ActiveRecord::VERSION::MAJOR >= 5 |
There was a problem hiding this comment.
There's a short-hand for this:
def migration_version
"[#{ActiveRecord::Migration.current_version}]" if ActiveRecord::Migration.respond_to?(:current_version)
end- Added blank line at the end of the migration generator - Added shorthand way of determining the current migration version
|
Thank you very much. I have made those changes now. |
|
The Travis failure looks to be happening due to a failure while bundling ruby 1.9.3. As far as I can tell, all that has changed since the last success is the rubygems version. I don't think this is something I can fix on my end, but if it is please let me know. |
robin850
left a comment
robin850
left a comment
There was a problem hiding this comment.
Thanks for updating ; just one change left and LGTM !
Yes, the fail on Travis is unrelated with your patch, no worries, we can handle that later !
| setup :prepare_destination | ||
|
|
||
| def active_record_version | ||
| "\\[#{ActiveRecord::VERSION::MAJOR}\\.#{ActiveRecord::VERSION::MINOR}\\]" if ActiveRecord::VERSION::MAJOR >= 5 |
There was a problem hiding this comment.
You can use ActiveRecord::Migration.current_version here as above.
|
Thanks very much. I've made that change now. |
|
@robin850 - do you know when this will be available as an update to the gem please? 👍 |
activerecord-session_store.gemspec
Outdated
| s.add_dependency('activerecord', '>= 4.0', '< 5.2') | ||
| s.add_dependency('actionpack', '>= 4.0', '< 5.2') | ||
| s.add_dependency('railties', '>= 4.0', '< 5.2') | ||
| s.add_dependency('activerecord', '>= 4.0', '<= 5.2') |
There was a problem hiding this comment.
Can you remove the <= 5.2 part? >= 4.0 is already sufficient.
5 participants
The sessions table migration that is generated currently always inherits from
ActiveRecord::Migration. This is no longer supported in Rails 5.1. This pull request will make it inherit fromActiveRecord::Migration[MAJOR.MINOR]for Rails 5+ applications.