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

Throw warning if DATABASE_URL isn't set #199

Merged
merged 6 commits into from
Jan 30, 2023

Conversation

palfrey
Copy link
Member

@palfrey palfrey commented Dec 14, 2022

Fixes #114

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

@codecov
Copy link

codecov bot commented Dec 14, 2022

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%> (ø)

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

@mattseymour
Copy link
Contributor

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
Copy link
Member Author

palfrey commented Dec 14, 2022

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 #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 palfrey changed the title Throw warning/exception if DATABASE_URL isn't set Throw warning if DATABASE_URL isn't set Jan 30, 2023
@palfrey
Copy link
Member Author

palfrey commented Jan 30, 2023

@ddelange @mattseymour I've updated this in line with the description now in #114 (comment) which I think is the right middle ground here. Thoughts? Can I merge?

Copy link

@ddelange ddelange left a comment

Choose a reason for hiding this comment

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

super many thanks!

@palfrey palfrey merged commit 47ac967 into jazzband:master Jan 30, 2023
@palfrey palfrey deleted the no-env-warning branch January 30, 2023 12:19
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.

Should we raise a KeyError if the env var is missing and default is None?
3 participants