search

bmcclure / drupal-commerce_fedex

Provides FedEx integration with Commerce Shipping.
5 stars 8 forks source link

Remove FedExServiceManager, move code into plugin #4

Closed bmcclure closed 7 years ago

bmcclure commented 7 years ago

Spoke with bojanz in Slack recently and he provided some feedback on the module.

One of the things he said was that we should get rid of the service since we're only using it in one place, and just put that code directly in the FedEx plugin.

I initially didn't want to bloat that plugin with connection details and wanted to abstract whatever's providing the FedEx objects for use, however I can see his point--a service isn't really necessary if this is the only place we're going to need to access it.

mdlutz24 commented 7 years ago

bojanz may be right about the service, I'm not sure. I'm inclined to keep it for now though. At some point, I need to set up shipping and tracking, and I'm not certain that that will get done through this module (though I would like to). It may be a separate custom module for my project, in which case, a service to access the fedex requestors would be handy. Even if it is in this module, it will be from other routes than just the plugin.

bmcclure commented 7 years ago

That's a good point. I agree that we should keep it for now and see how else we can make use of it. We can always roll it into the plugin itself later if it ends up not being used, but I think you're right that it could be handy for providing fedex request objects to other code outside the module as well.

bmcclure commented 7 years ago

The further we're going into expanding this module, I think the service makes sense to keep.