Fix unexpected Mail #categories, #categories= behavior

[tkbky]

This change makes Mail#categories= to only accept Category as its argument, otherwise, we would need to consider more cases if we allow it to accept different types of argument, and if not handle well, its behavior will be inconsistent.

I'm not sure if we should accept an array of Category as well, or probably we should just accept an array instead.

p/s: I've signed the CLA

[swifthand]

I agree that use of #to_json only to immediate fetch just one key feels like redundant work when you could instead ask the Category object directly via #category. However that does seem to be the way the whole library is tied together: Everything has a #to_json method, and many methods accepts any object which has a #to_json method, so long as it produces the appropriate Hash. Not my design decision to have made but I respect consistency when I see it.

Both the original interface (must have #to_json) and the interface of this proposed change (must have #name) are so small that a strict restriction to a Category object feels unnecessary.

Setting aside whether breaking the consistency of using #to_json everywhere, if all you need is a :name method, isn't it preferable to raise unless respond_to?(:name)?

A related issue is #95 which references all of the array assignment methods, converting the use of categories= from being a wrapper for push into the more explicit add_category.

[thinkingserious]

@tkbky,

Thank you for the pull request!

Thanks for the detailed feedback @swifthand.

I think #95 is the way to go for this particular issue.

On the bigger issue of the to_json design decision (among others), now is the chance to weigh in on a refactor. If you have a moment, please check out the Mail Helper Enhancement project.

[tkbky]

Agree with @swifthand on the unnecessary restriction.

@thinkingserious Yep, I think #95 (didn't notice that earlier) is the way to go. How should we proceed from here?

[thinkingserious]

@tkbky,

If you still want to tackle the issue, I suggest you just update this PR. Unless it's easier to just create a new one. Either is fine with me.

Thanks!

[thinkingserious]

I'm going to fork your pull request to make some further changes. Since this will be a breaking change there are few other things I'd like do while we have the hood up. Once I'm done, I'll make a pull request against your fork.

Here is what I will be doing:

1. Split the classes into their own files
2. Make sure we handle all cases of arrays consistently (example)

Thanks!

[tkbky]

@thinkingserious Great! I always hope the classes inside mail.rb could be live in their own files. And yes, the hashes must be handled consistently too (which I was planning in another PR but never mind now 😄 ). Looking forward to your changes. 😃

[SendGridDX]

All committers have signed the CLA.

[thinkingserious]

@tkbky,

Could you please take a moment to sign our new CLA? Now you just need to click this link and fill out a short form. There is no downloading and signing anymore :)

[tkbky]

@thinkingserious Done!

[thinkingserious]

Hello @tkbky,

Thanks again for the PR!

We appreciate your contribution and look forward to continued collaboration. Thanks!

Team SendGrid DX

[thinkingserious]

Thanks again to @mootpointer and @swifthand for your help on this one!