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

[Experimental] Upgrade To Rails 5.2 #1979

Closed
wants to merge 6 commits into from
Closed

[Experimental] Upgrade To Rails 5.2 #1979

wants to merge 6 commits into from

Conversation

dinesh-skyach
Copy link

Upgrade To Rails 5.2

@ysv ysv changed the title [WIP] Upgrade To Rails 5.2 [Experimental] Upgrade To Rails 5.2 Jan 15, 2019
@ysv ysv added v2.1 and removed v2.0 labels Jan 21, 2019
@ianeinser
Copy link

Bring it on dudes ASAP, for both v1.9 and v2.0 please.

@shal
Copy link

shal commented Feb 21, 2019

@dinesh-skyach Could you rebase the patch?

gem 'ruby-prof', '~> 0.17.0', require: false
gem 'annotate', '~> 2.7.4'
gem 'ruby-prof', '~> 0.17.0', require: false
gem 'spring', '~> 2.0.2'
Copy link

Choose a reason for hiding this comment

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

@dinesh-skyach Why do we need this gems: spring and spring-watcher-listen?

gem 'factory_bot_rails', '~> 4.11.1'
gem 'timecop', '~> 0.9.1'
gem 'rubocop-rspec', '~> 1.31.0', require: false
gem 'rails-controller-testing', '~> 1.0.4'
Copy link

Choose a reason for hiding this comment

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

I think later we should have a separate task to get rid of rails-controller-testing.
rails/rails#1895

@@ -35,7 +35,7 @@ class Order < ActiveRecord::Base

after_commit on: :update do
next unless ord_type == 'limit'
event = case previous_changes.dig('state', 1)
event = case state
Copy link

Choose a reason for hiding this comment

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

Maybe let's keep:

event = case after_save.dig('state', 1)

# path to your application root
APP_ROOT = File.expand_path('../../', __FILE__)
require 'fileutils'
include FileUtils
Copy link

Choose a reason for hiding this comment

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

I don't this is it necessary to include and require FileUtils. Just keep

Dir.chdir APP_ROOT do
  ...
end

require 'rails'

%w( active_record action_controller action_view sprockets ).each { |framework| require "#{framework}/railtie" }
require 'rails/all'
Copy link

Choose a reason for hiding this comment

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

I afraid amount of unused libs may speed down the application.

Copy link

Choose a reason for hiding this comment

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

Keep this:

require_relative 'boot'

require 'rails'

%w( active_record action_controller action_view sprockets ).each { |framework| require "#{framework}/railtie" }

@@ -0,0 +1 @@
nvyOF/IyR5uU8ouBSuMUepwoSYJbiAJ5uK+jXqp/Uy2s0BEktlGaRACEBxu+rAdiSNVAwKfm+/2ZSiBd4QXTZa8YGLOPTBjKhL9xoT8EFtLLngTp2KIz367L4dtCWZ2Cdv9Fqf4bnZtSKnPXk4oECN0OnsCmiUInZW4bVU9E5jFqTsAyzuD8P1KOkmFvjJAHxbuO/w1tuLiKW253DZhdtbW4LHABgUFagnPUym6jHnMec272zfjsrFhYSjFWeWGg1qIBwRow/l3f30HR93X0cXqCWv5CCgDOAElrxQEWJ3P5SnYEydt9t8IDvw9cSnmS5aPpzMxOqw7PB7bA4I8BW2tgmpi364I1UWSU1Nd2co3hOR5pmIpb7etbJJfC1jw/4Ai9qVQHeMqNXUDBO2uVR9JbKq7v3nnjP1A1--d4xxvUu/a12DL/sR--sJ1yQRmZFkqaGVQYVM1J7A==
Copy link

Choose a reason for hiding this comment

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

Why do we need this?

# Show full error reports and disable caching.
config.consider_all_requests_local = true
# Show full error reports.
config.consider_all_requests_local = true
Copy link

Choose a reason for hiding this comment

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

No need.

# yet still be able to expire them through the digest params.
config.assets.digest = true
# Suppress logger output for asset requests.
config.assets.quiet = true
Copy link

Choose a reason for hiding this comment

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

Try:

config.logger = ActiveSupport::Logger.new(STDOUT)
config.assets.quiet = true

@shal
Copy link

shal commented Feb 22, 2019

@dinesh-skyach @mod In my opinion this patch is very dangerous, it is too difficult to review all this changes and I think impossible to check everything.
I suggest to move slowly the version of rails.

Here is quote from rails main page:
https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#the-upgrade-process.

Let start from https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#upgrading-from-rails-4-2-to-rails-5-0 and then keep bumping the version slowly 😸

@shal
Copy link

shal commented Mar 4, 2019

Closed via #2095.

@shal shal closed this Mar 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants