Suggestions for README examples #30

Merged
merged 1 commit into from Nov 21, 2012

Conversation

Projects
None yet
2 participants
Contributor

nateberkopec commented Oct 11, 2012

We use MailView extensively in our app, but our usage pattern is a little different than suggested by the README, and I thought it might be useful to pass along the way we use MailView.

First - the syntax as written in the current README gives me (in Ruby 1.9.3 and Rails 3.1) a warning on startup:

warning: toplevel constant MailPreview referenced by Notifier::MailPreview

Taking MailPreview out of an ActionMailer class entirely and putting it in its own file gives you two advantages:

  1. No more warnings that make you feel like you're a bad Rubyist and need to go take a shower
  2. It makes more sense if you start referring to other mailers (like I do in the README changes I've made)

Also, I've included how we use Structs in our MailViews to act like stub objects. It seems like the easiest way to do it if your mails are simple and not using a lot of attributes from the object, or you don't have existing DB fixtures you can pull from.

I also added Ruby syntax highlighting to the examples, just for some prettiness.

Owner

packagethief commented Nov 21, 2012

Thanks Nate. I wouldn't recommend creating the preview in an initializer, though. Feels like the wrong place. Stick it in lib or app/mailers (we use app/mailers).

If you can remove that suggestion, as well as the defined?(MailView) check, I'll happily merge.

I get that people might want to only load this in development mode, and thus check for the MailView constant, but I don't think's worth getting into in the README. Let's just show the simplest possible setup.

Contributor

nateberkopec commented Nov 21, 2012

Cool!

I've updated as you asked and squashed my history, should be ready to merge.

packagethief added a commit that referenced this pull request Nov 21, 2012

Merge pull request #30 from nateberkopec/master
Suggestions for README examples

@packagethief packagethief merged commit ee13d17 into basecamp:master Nov 21, 2012

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