sendgrid / sendgrid-ruby

The Official Twilio SendGrid Led, Community Driven Ruby API Library
https://sendgrid.com
MIT License
625 stars 322 forks source link

dependencies #175

Closed jjb closed 7 years ago

jjb commented 7 years ago

Hi,

First of all, thanks for providing and maintaining what looks to be a full-featured client library! I'm just getting started and there are a couple things that stick out to me in the dependencies.

  1. the dependency on Sinatra and rack-protection. I'm guessing both of these are only needed for inbound processing. They each add dependencies of their own, so including the sendgrid-ruby gem in my projects adds a lot of unneeded gems. I saw in this comment that you will split the outgoing and incoming processing libraries in the future. Even before you do that, you can simply make the sinatra dependency optional. Put in the readme that if the user wants to do incoming processing, they have to add sinatra to the Gemfile. This isn't ideal because one can't control versions and such with as much precision and it might complicate the test suite setup a but, but overall it should be quite feasible.

  2. ruby_http_client - it looks like this is a standalone http client library developed by sendgrid? Why did you chose to make an http client library? There are several fantastic choices in ruby. Using an existing library would possibly reduce the weight of the dependencies, make it easier for users to work with your code and add new features, save you development time, let you benefit from the experience and history of those projects... this list goes on :)

John

thinkingserious commented 7 years ago

Hi @jjb,

Thanks for the feedback! Do you mind posting it here also as we are in the process of updating this SDK?

Regarding item 1, yes, we will be splitting this dependency out.

And for item 2, it's a thin wrapper over 'net/http' and 'net/https'. We split it out for two reasons: 1) to avoid any dependencies at all (which is another reason we need to break out that Sinatra dependency) and 2) we wanted to have a stand alone http client for simple generic API access. For the next version of this SDK, we may bring the http client back into the this SDK for simplicity. We would also entertain bringing in a third party http client dependency if the community desires it. Personally, I'd like it to be an option, unless there is a clear http client champion we can all agree on. Please express your position on the link above, if you don't mind. Thanks!

With Best Regards,

Elmer

jjb commented 7 years ago

@thinkingserious sure. but looks like the issue you linked me to is discussing the email sending api. i'm not sure my comments would be relevant there.

thinkingserious commented 7 years ago

@jjb,

Your comments are relevant in terms of the implementation, especially if its a breaking change. I'd like to minimize the amount of breaking changes.

Thanks!

With Best Regards,

Elmer

jjb commented 6 years ago

I'm disappointed that my simple suggestions haven't gotten any discussion, and that this issue was closed out because I made one comment in that other gigantic thread, which also hasn't gotten any activity since that comment in September.

jjb commented 6 years ago

some of these issues were also raised in #159, which is also closed

you don't need to make changes in a gigantic new version. "I'd like to minimize the amount of breaking changes." -- the purpose of semantic versioning is so users of a library can update to newer versions with confidence that they are getting bugfixes with no breaking changes. if there is a new major version, then they just need to look at the release notes and determing if the changes will affect them.

yes, it's nice if things come together to minimize the overhead of the user having to test with the new version. but the drawback is that needed changes don't come often enough (as is the case here).

thinkingserious commented 6 years ago

Thanks for checking back in @jjb! I really appreciate it.

This library is scheduled for enhancements on our backlog, please see this issue to track progress. I have added a link to this thread to make sure your feedback makes it way into the refactor.

I apologize for the massive delay. The reality for us is that of our 7 supported SDKs, this one gets the least traffic, which means I have limited resources to update it like I'd love to. That said, I'm doing my best to get it going ASAP, as I am really excited to integrate all the amazing feedback we've received so far.

Feedback like yours, helps me make the case to give this SDK more priority, so thank you again!

With Best Regards,

Elmer

jjb commented 6 years ago

Hi Elmer,

Thank you for your response. I'm not complaining about the speed by itself, I'm suggesting that you use a more agile approach to making changes.

But, maybe I should put my money where my mouth is and just submit PRs 😀

John

thinkingserious commented 6 years ago

Thanks for the PR @jjb!

I'm hoping to get your PR merged soon.

jjb commented 6 years ago

🆒