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

Modernize dj-database-url #151

Closed
wants to merge 1 commit into from
Closed

Conversation

dcwatson
Copy link

I think this PR addresses a number of outstanding issues, namely:

  • The ability to register custom backends/schemas (and simultaneously only support "blessed" Django backends by default)
  • The ability to pass arbitrary OPTIONS into config and parse
  • Cleaner customization of backend-specific options
  • Modernize the testing matrix
  • Blacken (and isort) the codebase

As I don't want to impose any more maintenance burden on you, consider this PR also as an offer to maintain this library. My original plan was to simply fork it and release under a new name, but figured it was worth offering. The current test suite is almost entirely unchanged (and passing - https://github.com/imsweb/django-dburl/actions/runs/1373144981) - it only drops pre-2.0 Django support, and registers the previously-supported backends:

https://github.com/imsweb/django-dburl/blob/modernize/test_dj_database_url.py#L12-L23

Happy to discuss further. I have a few projects at work that use this, so I'm invested in keeping it up to date, either by maintaining this library directly or maintaining my own fork under a new name.

@jacobian
Copy link
Collaborator

Thanks for the offer @dcwatson, I would like to take you up on it. Please see #152!

checks:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a checkout@v2 of this action you might want to "check out".

steps:
- uses: actions/checkout@v1
- name: Set up Python
uses: actions/setup-python@v1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly there's a setup-python@v2 as well.

fail-fast: false
matrix:
django-version: [2.0, 2.1, 2.2, 3.0, 3.1, 3.2]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Django 4.0 is out.

fail-fast: false
matrix:
django-version: [2.0, 2.1, 2.2, 3.0, 3.1, 3.2]
python-version: [3.6, 3.7, 3.8, 3.9]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to wrap it with quotes "3.10", otherwise it is interpreted as float 3.1.

@dcwatson
Copy link
Author

Thanks, we'll address these when we get the repository maintenance stuff squared away.

@s-i-l-k-e
Copy link

Hi, just enquiring about the state of this PR. Keen to help out where I can because I've noticed a few more outdated items in the code that'd be good to get squared away.

@dcwatson
Copy link
Author

dcwatson commented Apr 1, 2022

Now that we have the repository moved to its new home, I'll probably start merging and triaging this weekend. If you have specific fixes in mind that don't already have a PR or issue, by all means open up a new issue/PR.

@s-i-l-k-e
Copy link

Cheers, very much appreciated. Can do.

@mattseymour
Copy link
Contributor

Is is possible to break this PR down into smaller chunks to make reviewing easier. My concern is this touches a lot of things and while i get it may add improvement, it is sweeping as a change. As this package is use by so many people I am worried new bugs or issues could be introduced on something which is fundimentally stable.

@dcwatson
Copy link
Author

A lot of the noise in this PR was from code formatting and CI updates. I can rework it without those since they're already done, and see how things look.

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.

6 participants