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

Add --skip-action-mailer (or -M) to rails generate #18288

Merged
merged 1 commit into from Jan 2, 2015

Conversation

Projects
None yet
3 participants
@claudiob
Member

claudiob commented Jan 1, 2015

Stems from discussion at #5703 (comment) with @rafaelfranca

Show outdated Hide outdated ...ils/generators/rails/app/templates/config/environments/development.rb.tt
# Don't care if the mailer can't send.
config.action_mailer.raise_delivery_errors = false
<%- end -%>

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jan 1, 2015

Member

This will leave 2 blank lines on the generated file, we can fix that by including one of them in the unless condition.

@carlosantoniodasilva

carlosantoniodasilva Jan 1, 2015

Member

This will leave 2 blank lines on the generated file, we can fix that by including one of them in the unless condition.

assert_file "config/environments/production.rb" do |content|
assert_match(/# config\.action_mailer\.raise_delivery_errors = false/, content)
end
end

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jan 1, 2015

Member

without which skips? only action mailer? Maybe the test should explain that?

@carlosantoniodasilva

carlosantoniodasilva Jan 1, 2015

Member

without which skips? only action mailer? Maybe the test should explain that?

This comment has been minimized.

@claudiob

claudiob Jan 2, 2015

Member

@carlosantoniodasilva

Before this PR, generating an app without --skip-active-record, --skip-sprockets or --skip-test-unit would add require "rails/all" to config/application.rb.

This behavior was not tested anywhere in the test suite.

After this PR, --skip-action-mailer must also be absent in order for require "rails/all" to be in config/application.rb.

The code above is testing two things:

  • if none of those 4 skip options are present, then require "rails/all" will be in config/application.rb.
  • if the --skip-action-mailer is not present, then config.action_mailer... will be in the config/environments file

Do you think this is confusing? Would you rather have this split into two different testing methods?

@claudiob

claudiob Jan 2, 2015

Member

@carlosantoniodasilva

Before this PR, generating an app without --skip-active-record, --skip-sprockets or --skip-test-unit would add require "rails/all" to config/application.rb.

This behavior was not tested anywhere in the test suite.

After this PR, --skip-action-mailer must also be absent in order for require "rails/all" to be in config/application.rb.

The code above is testing two things:

  • if none of those 4 skip options are present, then require "rails/all" will be in config/application.rb.
  • if the --skip-action-mailer is not present, then config.action_mailer... will be in the config/environments file

Do you think this is confusing? Would you rather have this split into two different testing methods?

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jan 2, 2015

Member

It wasn't clear to me this intention under the test, because most of what it checks is action mailer related content. Anyway, thanks for your work on this 👍

@carlosantoniodasilva

carlosantoniodasilva Jan 2, 2015

Member

It wasn't clear to me this intention under the test, because most of what it checks is action mailer related content. Anyway, thanks for your work on this 👍

rafaelfranca added a commit that referenced this pull request Jan 2, 2015

Merge pull request #18288 from claudiob/add-skip-action-mailer
Add --skip-action-mailer (or -M) to rails generate

@rafaelfranca rafaelfranca merged commit ef877bb into rails:master Jan 2, 2015

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

@claudiob claudiob deleted the claudiob:add-skip-action-mailer branch Jan 2, 2015

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 11, 2016

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment