Return the name part of the email address in the 'to' hash

[r38y]

I need this for a project I am working on and I think it doesn't go against what the purpose of this gem is. Wow, that was a really awkward way to say that.

I am not thrilled with how I had to test this. I had to pass name: nil to a lot of them which I think is not what we want. It looks like the test data is setup in each test so it may be best to change

    @hash = {
      full: 'Bob <bob@example.com>',
      email: 'bob@example.com',
      token: 'bob',
      host: 'example.com',
      name: 'Bob',
    }

to

    @hash = {
      full: '<bob@example.com>',
      email: 'bob@example.com',
      token: 'bob',
      host: 'example.com',
      name: nil,
    }

You can see what I'm talking about here: r38y@db8dbee

Actually, I'm not sure one way is better than the other.

[r38y]

Er, I actually don't need this for what I'm doing. HOWEVER, I wish I could get to the from name. Would you be opposed to being able to access #from_name? Or maybe returning a hash like what happens with the to addresses?

A bit of explanation: I'm trying to be the most user friendly in my app in that if they reply to the email, and they haven't set their name, it will set their name to the from name. This is because others would have added them with just their email address and we want to backfill info if they never visit the website, just reply to emails.

[calebhearth]

I'm thinking something like this to clean this up a bit:

if includes_name?(full_address)
  parsed_address.merge!(name: extract_name)
end

That would probably go somewhere in the parse_address method, but this whole class seems to be in desperate need of a refactor to not be class-level methods.

[calebhearth]

Okay, pending an extraction of something like Address as you proposed, I still think I'd like a bit of cleanup here. Our style guide cautions against both return and if / unless surprises.

if name == full_address || name.empty?
  name
end

It may also be worth trying to not even set the key unless there is a "name", or to always return a string even if it empty. I'm somewhat torn between the two, but I'm leaning toward the empty string.

[r38y]

I'm not sure how an empty string would be better. Missing the key would return nil and most people will probably check #blank? anyways. However, I'll do whatever you prefer.

[calebhearth]

I'd be okay with including the parsed address information for from, configurable of course. I couldn't think of a use case when designing the interface originally, so decided against it.

Let's finish up the tasks of adding names and parsing from separately, and they can be merged later.

I'll finish looking at this later - it's pretty late.

[r38y]

What do you think about these next steps?

1. This PR. I don't think it is a breaking change and is fairly straight-forward
2. Refactor this part of the code. I was thinking it could be around an Address class, maybe something like https://gist.github.com/r38y/6156358
3. Update the gem to have the option of returning the parsed address hash for the from method. This would also add the same configuration options as the "to" option.

How does that sound?

[calebhearth]

Seems reasonable. There's a bit of cleanup I'd like to see on the diff before merging, though.

On Mon, Aug 5, 2013 at 10:36 AM, Randy Schmidt notifications@github.com
wrote:

What do you think about these next steps?

1. This PR. I don't think it is a breaking change and is fairly straight-forward
2. Refactor this part of the code. I was thinking it could be around an Address class, maybe something like https://gist.github.com/r38y/6156358
3. Update the gem to have the option of returning the parsed address hash for the from method. This would also add the same configuration options as the "to" option.

How does that sound?

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

[calebhearth]

What do you think of setting this up to be @hash[:full]. That removes the need to set :name in every test, and I think will result in a smaller change all around.

[r38y]

I just pushed an update but it seems like we should get rid of setting that one instance variable and use pieces from @hash in these tests. I think it is clearer.

[calebhearth]

Looks pretty good. Can you squash for me?

Ping @jayroh and @theycallmeswift

[jayroh]

@r38y is the merging of { name: 'Bob' } for a more explicit assertion? Like ... "know that email should equal the hash with the name as "Bob" (even though it's in the hash by default"

[jayroh]

@calebthompson mentioned that asserting email.name equals 'Bob' might be a more direct approach if my assumption above is the case .. or, to be more consistent, like how it is below with email.from.should eq @hash[:email]

[theycallmeswift]

+1 on @calebthompson's thoughts

[theycallmeswift]

lgtm

[jayroh]

@r38y just one thing, but overall - varr nice!

[r38y]

#73 was actually between this branch and that PR's branch. I think when you merged that in you got this one too?

[calebhearth]

So this change is actually to extract the portion of FULL NAME <full@name.com> portion of the email address. #73 extends the parsing to from addresses where it only effected to before. We still want these changes.

[r38y]

@calebthompson Yeah, turns out, I had submitted the PR to MYSELF when I was changing the PR branches r38y#1. I think I need to go back to bed.

Anywho. I rebased with master and changed some stuff to be in line with how you changed the tests in the configurable FROM.

I have a couple small tweaks to the Postmark adapter to make the name in the FROM work. Should that be a separate PR?

[calebhearth]

Yup.

[calebhearth]

Can you squash this up? I think the commit message needs to be a bit more descriptive as well.

Can you add your reasoning into the squashed commit? The PR description looks pretty good, so that would be a good starting place.

[r38y]

How's that?

[calebhearth]

Looks good.