Mail Helper Refactor - Call for Feedback

[thinkingserious]

Hello!

It is now time to implement the final piece of our v2 to v3 migration. Before we dig into writing the code, we would love to get feedback on the proposed interface.

[thinkingserious]

@elbuo8, @dibyadas, @w-, @belfazt, @mehronkugler, @yamamanx, @bwhmather-joivy, @andriisoldatenko, @jeffoneill, @Jakobovski

If you are tagged on this message, it means we are particularly interested in your feedback :)

If you don't have the time, no worries and my apologies for the disturbance.

If you do have the time, please take a look at the proposed helper upgrade above and let us know what you think. Any and all feedback is greatly appreciated.

Thanks in advance!

[jeffoneill]

Could you please elaborate on this? Does this relate to versions of the Python sendgrid-python library or of the Sendgrid REST API?

I'm currently using sendgrid-python 3.6.3 and have been using that version since February. I thought this library sent emails using Sendgrid v3 API that is documented here.

It seems I am already using a third version of both so it isn't clear to me what is being proposed.

[elbuo8]

I find having multiple properties to reference the same output can get confusing.

mail.to, mail.tos, mail.cc, mail.ccs.

I would make the interface accept either a string or a list and perform validations as needed.

[Jakobovski]

Adding the ability to send a request asynchronously would be nice

[thinkingserious]

@jeffoneill,

This relates particularly to the sendgrid-python library. It's purpose is to make interacting with the SendGrid v3 API easier, starting with the mail/send endpoint. We left off on providing direct access to all the SendGrid v3 API endpoints with some initial help around creating the request payload for mail/send, but we expose far too much detail. This proposal is designed to fix that.

You may also notice we are wrapping all the input data in objects. This is so that we can do client side validation easier.

@elbuo8,

Good call! Thanks for responding so quickly :)

@Jakobovski,

I would love to add that! That request is out of scope for this iteration, but we have your issue on our backlog. Issues gain priority in our backlog when we get additional +1's or a PR. When we receive a PR, that provides the biggest jump in priority.

[mbernier]

Hey, @elbuo8.

Hi ;)

[elbuo8]

@Jakobovski ++

👋 @mbernier @thinkingserious

[andriisoldatenko]

@thinkingserious when do you plan to start working on changes? Should we start now or wait some review of document? How do you plan to split work between contributors?

[dibyadas]

Hi everyone!

@thinkingserious
I am really sorry for not updating the PR. I really got extremely busy with my intern work and my academics. My mid sems begins tomorrow so I doubt whether I will have to time to complete my PR.
I would love to give collaborator access to my fork of this repo if someone's want to continue the work.

With Regards,
Dibya Prakash Das

[thinkingserious]

@andriisoldatenko,

The goal was to get at least 3 thumbs up from the community before proceeding with code. It looks like we are there now. That said, we have a few things on our backlog ahead of this project, so if you want to kick things off in a new fork from this branch, that would be awesome. The scope of this work is to implement the interfaces defined in the proposal.

@dibyadas,

No worries! I'm glad things are progressing with your intern work and academics. I'm sure we will be using elements of your work as we progress and you will receive due credit :)

[thinkingserious]

With regards to Python 2.6, I believe it's time to sunset support for that, since now it's no longer supported by the Python Core Team. Any thoughts on that?

[thinkingserious]

@andriisoldatenko,

If you decide to kick things off, please drop a note here. I will do the same so that either of don't end up duplicating efforts.

[diegoc-am]

The only thing that I thought looked confusing is what @elbuo8 said.

Everything else looks good

[thinkingserious]

@iandouglas,

If you have a moment, your sage advice would be appreciated :)

[thinkingserious]

Thanks @belfazt,

I will fix that :)

[iandouglas]

My only real complaint (aside from agreeing with @elbuo8 also) is this, which is found in several places in the proposal: except Exception as e:

While this is certainly better than just except: by at least naming some kind of exception to trap, this is still super generic and in my view, just as bad as a plain except: catch. I'd like to see better documentation and examples of the kinds of errors that would throw exceptions and best practices on handling them.

[thinkingserious]

@iandouglas,

Thanks for being awesome and responding so quickly!

Your wish is our command: sendgrid/python-http-client#17

I need to update the docs to reflect those changes.

[openly-retro]

I very much like the application of named arguments in the Mail helper and the other helpers. Overall, I can see I'll need fewer lines of code to implement things, so thank you.

Question: Is the structure of the dict that's being assembled behind the scenes by these helpers changing in any way? Or is it just the implementation of the helpers that's changing?

Having mail CCs outside of the To object is a bit confusing to me. I think it's logical to group/nest data per personalization together, rather than breaking out and grouping CC, BCCs together with a personalization reference (unless I'm reading it wrong).

I won't need Python 2.6 support over here, so +1 for moving forward.

[thinkingserious]

Hi @mehronkugler,

Yes, the implementation of the helpers is changing currently.

The structure of the dict is not changing this time around. For now, it will continue to closely mirror the underlying API.

Thank you very much for your feedback!

With Best Regards,

Elmer

[thinkingserious]

Implementation will be tracked in this issue.

[thinkingserious]

#769