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 as code owners Dec 3, 2020
@product-auto-label product-auto-label bot added the api: logging label Dec 3, 2020
@google-cla
Copy link

@google-cla 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 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 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

@bradykieffer bradykieffer commented Dec 3, 2020

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Dec 3, 2020
@daniel-sanche daniel-sanche added the kokoro:run label Dec 4, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:run label Dec 4, 2020
@daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Dec 4, 2020

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

@bradykieffer
Copy link
Contributor Author

@bradykieffer bradykieffer commented Dec 4, 2020

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

@daniel-sanche daniel-sanche commented Dec 4, 2020

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

@bradykieffer bradykieffer commented Dec 7, 2020

Awesome, I added tests + made get_response a required param

@daniel-sanche daniel-sanche added the kokoro:run label Dec 7, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:run label Dec 7, 2020
@daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Dec 7, 2020

Great, thanks for your contribution!

@daniel-sanche daniel-sanche merged commit e704f28 into googleapis:master Dec 7, 2020
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants