Skip to content
New issue

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

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

Already on GitHub? # to your account

Create necessary factories only when using HTTP transport #747

Merged
merged 1 commit into from Jan 24, 2019
Merged

Create necessary factories only when using HTTP transport #747

merged 1 commit into from Jan 24, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jan 24, 2019

This PR does the changes discussed in #741.

When setting a custom transport, the client builder will not create any factories used by the HTTP transport anymore. These prevented using a custom transport which did not rely on HTTPlug (because these relied on class discovery).

Closes #741.

@Jean85 Jean85 added this to the Release 2.0 milestone Jan 24, 2019
Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like a lot this change! Good job!

I would like to have a regression test for this, but I fear that's not possible.. @ste93cry WDYT?

@Jean85 Jean85 requested a review from ste93cry January 24, 2019 11:48
@ste93cry
Copy link
Collaborator

I would like to have a regression test for this

Some tests of the client builder class already checks whether the HTTP transport that is created uses the custom factories, so no need for a regression test. PR is fine for such change, the only thing I would like to explore (it's not blocking btw) is whether it makes sense to expose the set*Factory methods in the client builder. Since the client is agnostic of the transport, the builder should not know anything regarding a specific transport type which in this case is HTTP

@Jean85
Copy link
Collaborator

Jean85 commented Jan 24, 2019

I agree with the possibility of removing the factory setters. But we can address that in a separate PR.

Thanks @CharlotteDunois for this one, merging!

@Jean85 Jean85 merged commit a13e23c into getsentry:master Jan 24, 2019
@ghost ghost deleted the custom-transport branch January 24, 2019 15:04
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants