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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Remove keyword only argument for RequestsMiddleware #113

Merged
merged 3 commits into from Dec 7, 2020

Conversation

bradykieffer
Copy link
Contributor

@bradykieffer bradykieffer commented Dec 3, 2020

Remove keyword only arguments from request middleware. This causes
django to fail when attempting to load middleware. Django currently only
supports handlers being passed in as args.

Fixes #112 馃

@bradykieffer bradykieffer requested review from a team as code owners December 3, 2020 22:45
@product-auto-label product-auto-label bot added the api: logging Issues related to the googleapis/python-logging API. label Dec 3, 2020
@google-cla
Copy link

google-cla bot commented Dec 3, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

馃摑 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

鈩癸笍 Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Dec 3, 2020
Remove keyword only arguments from request middleware. This causes
django to fail when attempting to load middleware. Django currently only
supports handlers being passed in as args.
@google-cla
Copy link

google-cla bot commented Dec 3, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

馃摑 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

鈩癸笍 Googlers: Go here for more info.

@bradykieffer bradykieffer changed the title Remove keyword only argument for RequestsMiddleware fix: Remove keyword only argument for RequestsMiddleware Dec 3, 2020
@bradykieffer
Copy link
Contributor Author

@googlebot I signed it!

@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Dec 3, 2020
@daniel-sanche daniel-sanche added the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 4, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 4, 2020
@daniel-sanche
Copy link
Contributor

Good catch, thanks! Do you think you could add a unit test to catch this going forward?

@bradykieffer
Copy link
Contributor Author

Of course @daniel-sanche! I broke my rule about always pushing up a patch with a test, especially when there's a bug. I commented on #112:

A colleague of mine pointed out that this issue is related to the 2.0.0 update. Looking at Django's docs for 3.1 and 2.2 it doesn't seem like get_response is an optional, we could default it to non-null to fix this issue.

Do you know if code gen will cause this to fail going forward? If so, I think it's safe make get_response a required param

@daniel-sanche
Copy link
Contributor

Of course @daniel-sanche! I broke my rule about always pushing up a patch with a test, especially when there's a bug. I commented on #112:

Perfect, thanks!

A colleague of mine pointed out that this issue is related to the 2.0.0 update. Looking at Django's docs for 3.1 and 2.2 it doesn't seem like get_response is an optional, we could default it to non-null to fix this issue.

Yeah, I just took a look at the django documentation and that seems to be the case, so that sounds like a good idea.

Do you know if code gen will cause this to fail going forward? If so, I think it's safe make get_response a required param

The code generator won't be an issue here, it should only impact /proto, /services, and /types. This file is pretty stand-alone

@bradykieffer
Copy link
Contributor Author

Awesome, I added tests + made get_response a required param

@daniel-sanche daniel-sanche added the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 7, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 7, 2020
@daniel-sanche
Copy link
Contributor

Great, thanks for your contribution!

@daniel-sanche daniel-sanche merged commit e704f28 into googleapis:master Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the googleapis/python-logging API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keyword only args causes RequestMiddleware to fail on Django app load
3 participants