This repository has been archived by the owner. It is now read-only.

Simplified the README example. #76

Merged
merged 1 commit into from Jan 7, 2013

Conversation

Projects
None yet
3 participants
Contributor

jrgifford commented Jan 7, 2013

Instead of getting tricky with .tap, we should be keeping it simple.

def update
  redirect_to current_account.people.find(params[:id]).tap { |person|
    person.update_attributes!(person_params)
  }
end

That's the current README example.

This can (and probably should) be simplified to

def update
  person = current_account.people.find(params[:id])
  person.update(person_params)
  redirect_to person
end

They do the exact same thing from what I can tell.

(thanks @steveklabnik for helping me figure this out)

Simplified the README example.
Instead of getting tricky with .tap, we should be keeping it readable.
Member

steveklabnik commented Jan 7, 2013

This came out of a discussion @jrgifford and I had. I don't think using #tap here is a good idea; the feature is already new, and it leads to assuming you need #tap/#update_attributes in general, since it's the only code example:

  def create
    @article = Article.find(params[:article_id])
    @comment = @article.comments.create.tap {|comment|
      comment.update_attributes!(comment_params)}
    redirect_to article_path(@article)
  end

So I'm obviously 👍, but I figured I'd let someone else double-check and then merge.

Owner

rafaelfranca commented Jan 7, 2013

:shipit:

steveklabnik added a commit that referenced this pull request Jan 7, 2013

@steveklabnik steveklabnik merged commit 71078ec into rails:master Jan 7, 2013

1 check passed

default The Travis build passed
Details
Member

steveklabnik commented Jan 7, 2013

Your wish is my command. 🎊

Thank you for your contribution, @jrgifford

Contributor

jrgifford commented Jan 7, 2013

My pleasure @steveklabnik. 😀

@jrgifford jrgifford deleted the jrgifford:fix-readme-example branch Jan 7, 2013

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