-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
Generic backend registration #171
base: master
Are you sure you want to change the base?
Conversation
@alexanderGerbik thanks for the submission, we have had two very similar yet competing PRs come in within a couple of hours of each other. Can I direct you to #170 and see if the changes you made can work along side this PR. 📓 I have put a similar message on #170 |
dj_database_url.py
Outdated
schemes = schemes or [backend.rsplit(".")[-1]] | ||
schemes = [schemes] if isinstance(schemes, str) else schemes | ||
for scheme in schemes: | ||
if scheme not in _seen_schemes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this can't just be if scheme not in ENGINE_SCHEMES
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Fixed.
From what I can tell, this builds off of #151 and changes the options mechanism into a decorator to adjust the parsed settings. Given that it has more tests and an updated README, I'm inclined to start with this PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #171 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 76 131 +55
Branches 18 33 +15
=========================================
+ Hits 76 131 +55 ☔ View full report in Codecov by Sentry. |
@mattseymour any chances of this PR being merged? It addresses all the other opened PR's as well as some of opened issues. |
dfc2241
to
870f7f2
Compare
bb99c49
to
6927726
Compare
6927726
to
a823de4
Compare
This is quite a sweeping change. It would be good if we could get a couple of sets of eyes over the ammendments just to make sure we cover off as much as possible. From the outset its looking good. It will just be a case of really giving it a good test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks useful and future-proof.
eafc7c9
to
bd9a8fa
Compare
@mattseymour @alanjds |
8ccb888
to
b27bd63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get this merged!
Add support for custom backend registration with optional config post-processing.
Add support for passing arbitrary django database settings as kwargs.Add feature description to README
and make README more clear.Extract tests with deprecated options to separate suite to simplify further removal.