jazzband / dj-database-url

Use Database URLs in your Django Application.

https://pypi.org/project/dj-database-url/

BSD 3-Clause "New" or "Revised" License

1.48k stars 205 forks source link

Throw warning if DATABASE_URL isn't set #199

Closed palfrey closed 1 year ago

palfrey commented 1 year ago

Fixes https://github.com/jazzband/dj-database-url/issues/114

Along the way, to make the test cleaner, it fixes all the explicit "set os.environ" calls with mock.patch work instead.

codecov[bot] commented 1 year ago

Codecov Report

Merging #199 (f8468dc) into master (9c1ebdc) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #199   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           66        69    +3     
  Branches        15        16    +1     
=========================================
+ Hits            66        69    +3

Impacted Files	Coverage Δ
dj_database_url.py	`100.00% <100.00%> (ø)`

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

mattseymour commented 1 year ago

Whilst I get what this PR is trying to do I do not think adding in extra parameters is the correct way to do this. It seems like a messy API design.

We also need to be wary that a user might want the default to be None in which case you have changed the api.

I think this needs to be thought over before proceeding.

palfrey commented 1 year ago

Whilst I get what this PR is trying to do I do not think adding in extra parameters is the correct way to do this. It seems like a messy API design.

Agreed, but I thought that was where consensus in https://github.com/jazzband/dj-database-url/issues/114 had gotten to, hence adding it. I can remove that bit if you want.

We also need to be wary that a user might want the default to be None in which case you have changed the api.

If they do that, then it won't configure a database at all. I'd say that the working assumption for this library is "users always want a DB", and that warnings should flag if we're failing to do that. So, anything other than None is a perfectly good value and should be fed to parse, but None should warn IMHO.

Thoughts?

palfrey commented 1 year ago

@ddelange @mattseymour I've updated this in line with the description now in https://github.com/jazzband/dj-database-url/issues/114#issuecomment-1406861561 which I think is the right middle ground here. Thoughts? Can I merge?