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

Add _mailer suffix to generated file names. #18074

Merged
merged 1 commit into from Jan 6, 2015
Merged

Conversation

@caike
Copy link
Contributor

@caike caike commented Dec 17, 2014

It is somewhat confusing that controllers and jobs follow this convention, but mailers don't.

@matthewd
Copy link
Member

@matthewd matthewd commented Dec 17, 2014

Given that we're changing the existing behaviour, perhaps we could detect and silently ignore any trailing _mailer in the supplied name?

So,

rails generate mailer Notifications
rails generate mailer NotificationsMailer

would both generate the same thing?

If we're asserting that FooMailer is the "normal" choice of name, then presumably people are currently more likely to run the latter of those commands.

@caike
Copy link
Contributor Author

@caike caike commented Dec 17, 2014

Excellent point! I'll work on that next.

@caike
Copy link
Contributor Author

@caike caike commented Dec 18, 2014

Please let me know if there's a better way to prevent duplicate suffixes like I'm doing here.

I feel like there should be..

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 2, 2015

The implementation looks good to me. @dhh what do you think about the convention change?

@dhh
Copy link
Member

@dhh dhh commented Jan 2, 2015

👍

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 2, 2015

Great! @caike mind to add a CHANGELOG entry and squash your commits?


protected
def file_name
@_file_name ||= super.gsub(/\_mailer/i, '')

This comment has been minimized.

@JuanitoFatas

JuanitoFatas Jan 2, 2015
Contributor

Can we use sub here?

@_file_name ||= super.sub(/\_mailer\.rb$/i, '')

This comment has been minimized.

@caike

caike Jan 6, 2015
Author Contributor

I've tried your suggestion @JuanitoFatas , but looks like using sub this way doesn't account for removing duplicates. See the failure on MailerGeneratorTest#test_mailer_suffix_is_not_duplicated at https://travis-ci.org/rails/rails/jobs/46081505

Following the same naming convention used in
controllers and jobs.
rafaelfranca added a commit that referenced this pull request Jan 6, 2015
Add _mailer suffix to generated file names.
@rafaelfranca rafaelfranca merged commit 5d28c6b into rails:master Jan 6, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 6, 2015

❤️ 💚 💙 💛 💜

@carlosantoniodasilva
Copy link
Member

@carlosantoniodasilva carlosantoniodasilva commented Jan 7, 2015

💚💛❤️💙💜

@@ -25,55 +25,55 @@ def test_application_mailer_skeleton_is_created

def test_mailer_with_i18n_helper
run_generator
assert_file "app/mailers/notifier.rb" do |mailer|
assert_file "app/mailers/notifier_mailer.rb" do |mailer|
assert_match(/en\.notifier\.foo\.subject/, mailer)
assert_match(/en\.notifier\.bar\.subject/, mailer)

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jan 7, 2015
Member

Shouldn't the i18n path for the subject change as well?

This comment has been minimized.

@caike

caike Jan 9, 2015
Author Contributor

Good catch! 👍
New PR submitted

@caike
Copy link
Contributor Author

@caike caike commented Jan 9, 2015

Thanks for catching those @y-yagi ! 👍

rafaelfranca added a commit that referenced this pull request Feb 18, 2015
follow up to #18074
y-yagi added a commit to y-yagi/slim-rails that referenced this pull request Oct 7, 2016
In Rails 5, was forced as `_mailer` suffix is set.
Ref: rails/rails#18074
y-yagi added a commit to y-yagi/slim-rails that referenced this pull request Oct 7, 2016
In Rails 5, was forced as `_mailer` suffix is set.
Ref: rails/rails#18074
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.