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

Allow overriding context for Django Middleware. #73

Merged

Conversation

sjoerdjob
Copy link
Contributor

By subclassing django.HoneyMiddlewareBase, it is now possible to
change the context that gets sent to Honeycomb.

See #72.

@sjoerdjob
Copy link
Contributor Author

Unsure if the change I made is desirable for the general project, but this way I think it is easier to override the "what gets sent" from the "when does it get sent".

By subclassing `django.HoneyMiddlewareBase`, it is now possible to
change the context that gets sent to Honeycomb.

See honeycombio#72.
@sjoerdjob sjoerdjob force-pushed the django-allow-overriding-context branch from 880d934 to 412c082 Compare July 29, 2019 14:58
@eanakashima eanakashima requested review from tredman and asdvalenzuela and removed request for tredman July 29, 2019 16:37
@tredman
Copy link
Contributor

tredman commented Jul 30, 2019

This LGTM, thanks for the PR! As it's still marked draft I won't merge it yet in case you had some other changes you were looking to add. Since we don't have automated testing of the django middleware here (yet), when I merge, I'll test it against a local environment before cutting a new release.

@sjoerdjob
Copy link
Contributor Author

I only marked it as draft because I was wondering whether you would like to apply the same change to all the other middleware as well, seeing as I would think it would be generally desirable.

I'll mark it as "ready for review" so that when you want to merge it's good to go.

@sjoerdjob sjoerdjob marked this pull request as ready for review July 31, 2019 09:31
@sjoerdjob
Copy link
Contributor Author

Ok, after thinking about it more, I do have one last change I'd like to make: I'd like to add request as a parameter to get_response_context.

@tredman
Copy link
Contributor

tredman commented Aug 2, 2019

I only marked it as draft because I was wondering whether you would like to apply the same change to all the other middleware as well, seeing as I would think it would be generally desirable.

ah, makes sense! Yeah I think it'd be worthwhile. Did you have interest in adding that to this PR? If not I can spend a little time adding it before cutting the next release.

@sjoerdjob
Copy link
Contributor Author

I myself do not currently have the availability to do that myself in the upcoming week. If you could do that it would be great.

@tredman
Copy link
Contributor

tredman commented Aug 5, 2019

I'll reply back here when the new release is out. Should have this ready by tomorrow.

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.

2 participants