Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/support dependency injection #115

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

ericnewton76
Copy link
Owner

Implemented constructor based dependency injection

Created new type Google.Maps.Services that perform default (poor-mans-DI-framework) injection for ease of use.

@ericnewton76
Copy link
Owner Author

@richardthombs was looking for a review and comments of this.

This is a change to the public API to better support dependency injection and to fix a couple of issues related to testing and mocking (ie by removing static GoogleSigned property) and the like.

@ericnewton76
Copy link
Owner Author

Closes #95

@richardthombs
Copy link
Contributor

I like the idea of abstracting the repeated GetResponse and GetResponseAsync into an abstract base class, but I think the the interfaces that presumably you intend to use for DI are really complicated and too low level.

If I wanted to mock say the GeocodingService, I'd far rather interact with an IGeocodingService interface than an IWhatever<IGeocodingRequest,IGeocodingResponse>.

I think trying to put an interface on the request and response objects is going too far. I've not seen anything else do that and I struggle to think of any use case where that would be useful.

@ericnewton76
Copy link
Owner Author

ericnewton76 commented Nov 11, 2017 via email

@richardthombs
Copy link
Contributor

I don't know... this breaks API compatibility for no obvious reason.

I think that backoff could be added directly into MapsHttp and then enabled by setting a service property. Eg: new GeocodingService { Retries = 10 } or something similar. If the property defaulted to 1, then both the API and the default behaviour would be entirely backward compatible.

We can argue back and forth about whether it would be better to refactor the code internally so that it uses DI, but if it involves breaking the API then I think it is better pushed to version 2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants