Added the forcedefault option #7

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

I added the forcedefault or f option. This allows you to force a gravatar to any of the available defaults even if the user has a defined gravatar image--it will be ingored.

I don't really know what I'm doing, and I've only been doing Ruby for 3 days... but the commit is fairly small, and it works for me.

More of the f option can be found here: http://en.gravatar.com/site/implement/images/ under "Force Default"

Owner

sinisterchipmunk commented Jan 12, 2012

Thanks very much for this pull request! However, there are a few minor problems. First of all (and the only real reason I didn't merge this), there is no corresponding unit test. I'm big on tests. :) Also, it looks like you've mistakenly committed the gem file, and I'm also not a big fan of bumping version before release -- otherwise we end up skipping version numbers and other kinds of madness.

So, I've implemented your changes separately in a0cb351.

As for installing the gem, you can do it the traditional way with gem install path/to/gravatar.gem --local or with the project's Rake task using rake install.

Yeah, sorry about that. I read the pull-request notes in the README but belatedly. I'm also very new to git, Ruby, RoR, and consoles, so doing unit tests was beyond me :D

Cheers for implementing the change.

Owner

sinisterchipmunk commented Jan 12, 2012

No worries, it is still a valuable contribution since it'd have gone unnoticed otherwise. I updated the contributors at the bottom of the readme to include you.

Cheers Colin!

I have a small question, if you don't mind. I couldn't find anything in the
documentation on how your gem is intended for use. Do we use it in the view
or the controller? I am using it in my views as such:

<%= link_to image_tag Gravatar.new(u.email).image_url() %>

The documentation examples all appear to use assign a variable, though,
leading me to think its intended for use in the controller. Any
clarification would be appreciated.

Thanks again,
Mohamad

On Thu, Jan 12, 2012 at 11:54 AM, Colin MacKenzie IV <
reply@reply.github.com

wrote:

No worries, it is still a valuable contribution since it'd have gone
unnoticed otherwise. I updated the contributors at the bottom of the readme
to include you.


Reply to this email directly or view it on GitHub:
#7 (comment)

Owner

sinisterchipmunk commented Jan 13, 2012

Well, what technically works is a different story from best practices. You've already discovered that there are a hundred ways it'll work, so let's discuss best practices.

Following the "Rails Way", the goal should always be to keep the models fat, the controllers skinny, and the views logic-less. If you absolutely must use logic in the views, put that logic into a helper and call it from the view, to keep clutter down.

With this principle in mind, I'd say treat the Gravatar instance as a model. Instantiate it from the controller, and use it in the view as if you were embedding the user's actual email address (or any other attribute) into the page. Example:

# controller code
def show
  @user = User.find 1 # just some arbitrary user
  @gravatar = Gravatar.new(@user.email)
end

# view code
<%= link_to image_tag @gravatar.image_url %>

Having written all that, it becomes apparent that we could make this a bit more natural. It stands to reason (and shows in the code) that "a user has a gravatar". Therefore, Gravatar becomes an attribute of User. Let's fix this:

# controller code
def show
  @user = User.find 1
end

# model code
class User < ActiveRecord::Base
  # ...

  def gravatar
    @gravatar ||= Gravatar.new(email)
  end
end

# view code
<%= link_to image_tag @user.gravatar.image_url %>

This is much more conventional, and keeps the code DRY (Don't Repeat Yourself!) because you don't have to constantly create a new Gravatar instance throughout your controllers.

As an aside, the ||= syntax means "if it exists already, return it; otherwise, create it, assign it, and then return it."

Colin, a big thank you for this great explanation. It makes a lot of sense. I need to think more OO, I suppose.

Cheers!

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