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

Update Rails to 5.1 #847

Merged
merged 6 commits into from
Jul 6, 2017
Merged

Update Rails to 5.1 #847

merged 6 commits into from
Jul 6, 2017

Conversation

delphaber
Copy link
Contributor

Update Rails to 5.1

(adds support for webpack too)

@@ -229,7 +229,7 @@

expect(bin_setup).to include("PARENT_APP_NAME=#{app_name.dasherize}-staging")
expect(bin_setup).to include("APP_NAME=#{app_name.dasherize}-staging-pr-$1")
expect(bin_setup).to include("heroku run rake db:migrate --exit-code --app $APP_NAME")
expect(bin_setup).to include("heroku run rails db:migrate --exit-code --app $APP_NAME")

Choose a reason for hiding this comment

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

Line is too long. [91/80]

@@ -32,7 +32,7 @@
it "includes the bundle:audit task" do
Dir.chdir(project_path) do
Bundler.with_clean_env do
expect(`rake -T`).to include('rake bundle:audit')
expect(`rails -T`).to include('rails bundle:audit')

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

desc: "Skip system test files"

class_option :skip_turbolinks, type: :boolean, default: true,
desc: "Skip turbolinks gem"

Choose a reason for hiding this comment

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

Align the elements of a hash literal if they span more than one line.

@@ -29,6 +29,12 @@ class AppGenerator < Rails::Generators::AppGenerator
class_option :skip_test, type: :boolean, default: true,
desc: "Skip Test Unit"

class_option :skip_system_test, type: :boolean, default: true,
desc: "Skip system test files"

Choose a reason for hiding this comment

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

Align the elements of a hash literal if they span more than one line.

@@ -227,7 +219,7 @@ def use_postgres_config_template
end

def create_database
bundle_command 'exec rake db:create db:migrate'
bundle_command 'exec rails db:create db:migrate'

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@delphaber
Copy link
Contributor Author

Ouch... hound is barking out loud!

@@ -229,7 +229,9 @@

expect(bin_setup).to include("PARENT_APP_NAME=#{app_name.dasherize}-staging")
expect(bin_setup).to include("APP_NAME=#{app_name.dasherize}-staging-pr-$1")
expect(bin_setup).to include("heroku run rake db:migrate --exit-code --app $APP_NAME")
expect(bin_setup).to(
include("heroku run rails db:migrate --exit-code --app $APP_NAME")

Choose a reason for hiding this comment

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

Put a comma after the last parameter of a multiline method call.

@@ -229,7 +229,9 @@

expect(bin_setup).to include("PARENT_APP_NAME=#{app_name.dasherize}-staging")
expect(bin_setup).to include("APP_NAME=#{app_name.dasherize}-staging-pr-$1")
expect(bin_setup).to include("heroku run rake db:migrate --exit-code --app $APP_NAME")
expect(bin_setup).to(
include("heroku run rails db:migrate --exit-code --app $APP_NAME")

Choose a reason for hiding this comment

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

Put a comma after the last parameter of a multiline method call.

@@ -229,7 +229,9 @@

expect(bin_setup).to include("PARENT_APP_NAME=#{app_name.dasherize}-staging")
expect(bin_setup).to include("APP_NAME=#{app_name.dasherize}-staging-pr-$1")
expect(bin_setup).to include("heroku run rake db:migrate --exit-code --app $APP_NAME")
expect(bin_setup).to(
include("heroku run rails db:migrate --exit-code --app $APP_NAME")

Choose a reason for hiding this comment

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

Put a comma after the last parameter of a multiline method call.

@@ -229,7 +229,9 @@

expect(bin_setup).to include("PARENT_APP_NAME=#{app_name.dasherize}-staging")
expect(bin_setup).to include("APP_NAME=#{app_name.dasherize}-staging-pr-$1")
expect(bin_setup).to include("heroku run rake db:migrate --exit-code --app $APP_NAME")
expect(bin_setup).to(
include("heroku run rails db:migrate --exit-code --app $APP_NAME")

Choose a reason for hiding this comment

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

Put a comma after the last parameter of a multiline method call.

@delphaber
Copy link
Contributor Author

I didn't change config/puma.rb, but I noticed that in Rails 5.1 they changed it a little (see railsdiff http://railsdiff.org/5.0.4/5.1.1 ).

Heroku guide on puma (https://devcenter.heroku.com/articles/deploying-rails-applications-with-the-puma-web-server) has not been updated since 2016 though.

So I don't know if config/puma.rb needs an update, because I'm not a big expert of puma on heroku :D. Any idea?

@tute
Copy link
Contributor

tute commented Jun 26, 2017

The puma diff has only comment changes, should be fine as is.

cc @nickcharlton for review, please! Thank you.

@delphaber
Copy link
Contributor Author

I noticed that rails 5.1 now:

Initialize Git repo when generating new app, if option --skip-git is not provided. (Pull Request)

So I think I can remove the init_git step in suspenders. Will work on it this evening (CEST time)

@delphaber
Copy link
Contributor Author

Ok I did it. The git repository is now created by Rails. --skip-git still exists in Rails.

@delphaber
Copy link
Contributor Author

Have you changed hound settings? :D I'm pretty sure I had to use double quotes for strings before!

@nickcharlton
Copy link
Member

@delphaber I think that the organisation config got lost on Hound and had to be reset. If you pushed again it'll rerun and be happy.

@delphaber
Copy link
Contributor Author

@nickcharlton I pushed an empty commit, but those hound comments did not disappear from the PR. I think we can ignore them anyway, since hound check is ok!

@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@thoughtbot thoughtbot deleted a comment from houndci-bot Jul 6, 2017
@nickcharlton
Copy link
Member

@delphaber Thanks! I went through, confirmed by eye and then deleted the invalid comments.

Going to merge now!

@nickcharlton nickcharlton merged commit 8c39efc into thoughtbot:master Jul 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants