-
Notifications
You must be signed in to change notification settings - Fork 271
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
Unify handling of bearer token security scheme. #515
Conversation
Codecov Report
@@ Coverage Diff @@
## master #515 +/- ##
=======================================
Coverage 98.62% 98.62%
=======================================
Files 58 59 +1
Lines 5951 5980 +29
=======================================
+ Hits 5869 5898 +29
Misses 82 82
Continue to review full report at Codecov.
|
Hey @ngnpope, thanks for the PR, but i am leaning against it for the following reasons:
|
Fixes `JWTScheme` to behave similar to the implementation of `TokenScheme` and `SimpleJWTScheme`. Also adds additional tests.
6e78215
to
3e3479d
Compare
I understand. I was looking at consistency from a different angle w.r.t. the output generated. I pushed an updated version that uses a separate mixin for composition instead of inheritance, de-cluttering the extension base class. I also updated the tests to reduce the churn significantly. I don't know whether it's more acceptable... |
apologies @ngnpope, i was too quick to discard this. I missed that this is actually used 3 times. your change is fine, but the placement, namings, and partially the tests did not sit 100% right with me. i took the liberty to rewrite this a bit differently. hope this is fine with you. thank you for being persistent here. |
Thanks 🙂 Yes, I was hoping that this approach would help avoid it diverging again in the future.
No worries. You know how your project is structured best so 👍🏻. |
Fixes
JWTScheme
to behave similar to the implementation ofTokenScheme
andSimpleJWTScheme
. Also adds additional tests.This follows on from #508 (comment).