search

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.49k stars 205 forks source link

allow TEST settings to be passed into a db config #116

Closed Brodan closed 1 year ago

Brodan commented 5 years ago

currently DB configs do not allow for a 'TEST' configuration to be passed in, this PR will allow it.

Fixes #102.

Brodan commented 5 years ago

@sigmavirus24 can you or someone please take a look at this. thanks!

sigmavirus24 commented 5 years ago

I no longer maintain this package

Brodan commented 5 years ago

Does anyone maintain it? If not I would be willing to take it over. Please let me know, @sigmavirus24.

sigmavirus24 commented 5 years ago

I can't answer that or give you those rights, please stop mentioning me on this

Brodan commented 5 years ago

@kenneth-reitz Can you assist?

Brodan commented 5 years ago

Bump, kinda sucks that no one will look at this. I'm willing to take over the repo, still seems relevant and like a lot of people use it.

MattOates commented 5 years ago

Some feedback on this PR yourtest={} is config.update is at the top level of the DB config. It would be cleaner and more specific if your code did config.update({'TEST': test}) to make sure it's only altering the TEST settings. Otherwise it's not really anything to do with test, but a generic dict you can patch anything with.

Brodan commented 5 years ago

Hey @jacobian! I noticed this repo just changed owners, think you can take a look at this soon?

jacobian commented 5 years ago

@Brodan I will try, no promises though. I've taken this over as an interim thing (see #120) and am not yet totally sure what my level of commitment I'm up for.

On a more meta note: if this issue is indicative of how you engage with open source, I'd suggest you think carefully about your approach. I know it can be super-frustrating having an issue you care about linger! But you have to remember that the vast majority of open source is maintained by people doing work in their spare time. Vanishingly few of us are paid for our time, and most of us have a ton of other commitments. We work on this stuff when we can, but it's often not a lot of time.

Given that context, I hope you'll see why pinging people to take a look at your work isn't particularly effective. It adds to an already stressful situation, and can lead to burnout. It's a counter-productive spiral: added stress means that working on the project doesn't seem like much fun, which adds more stress, and down we go. Paradoxically, the more you ask, the less folks want to help.

I know your intention here is good: clearly you're not intending to be a pest! But I hope you'll think a bit about how the repeated pings look from a maintainer's POV.

Again, I'll see if I can get around to this shortly, but I can't make any promises, sorry.

Brodan commented 5 years ago

Thank you for your feedback @jacobian. While I feel that my engagement in this thread was respectful and not out of the ordinary, I understand your points and will try to be more mindful in the future.

There's no rush to getting this merged of course, but it's nice to know that someone will eventually look at this, as it's something that can benefit more than just myself based on what I've seen.

jacobian commented 5 years ago

@Brodan thanks for understanding.

OK, I've taken a look, and this mostly looks good. I've got two small changes that I'd like to request before merging:

  1. The documentation (README) needs to be updated explaining that this is possible.
  2. Very minor, but I think the argument should be test_name rather than just test. At first I wasn't sure if I should just pass the database name, or the whole test dictionary (e.g. {"test": "default"}); renaming it to test_name (or even test_database_name?) makes this more clear.

Thanks!

Brodan commented 5 years ago

@jacobian Thank you for the quick review!

test is actually intended to be a dictionary, since more than just NAME can be configured. I updated the variable name to test_options. Hopefully that's a little better.

I also added a note about this new option in the README under 'Usage'. Let me know your thoughts. I can make changes again if necessary.

mattseymour commented 5 years ago

I think naming it test_options is a good shout. This change looks good to me.

Brodan commented 5 years ago

bump!

Brodan commented 4 years ago

Bumping this again. Would love if it could be looked at as I could use this feature at work and others have also expressed interest! It's quite a small PR and it's been over a year since it opened.

mattseymour commented 4 years ago

This PR code seems to cover all the points raised above and in the various issues. I think its probably at a good point to merge into the main branch if no one has any objections.

Brodan commented 4 years ago

Bump, is there anything I can do to help get this merged? It's over a year old. I'd rather not have to fork and package this on my own. This would solve an enormous pain-point I'm having at work.

Brodan commented 4 years ago

Bumping again. @jacobian Is there anything blocking this from being merged? It's been open for nearly two years...

Brodan commented 3 years ago

@jacobian bumping again

Brodan commented 2 years ago

@jazzband now that this has been transferred over can anyone finally take a look at this and get it over the finish line?

palfrey commented 1 year ago

@jazzband now that this has been transferred over can anyone finally take a look at this and get it over the finish line?

I've just joined jazzband, mostly to help with this project a bit :) If you can fix the conflicts, I'm happy to get this merged in.

Brodan commented 1 year ago

@palfrey Done! Thanks so much for actually taking a look. Lemme know if my update is sufficient

codecov[bot] commented 1 year ago

Codecov Report

Merging #116 (c9253a6) into master (e9cb03d) will increase coverage by 0.41%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
+ Coverage   86.15%   86.56%   +0.41%     
==========================================
  Files           1        1              
  Lines          65       67       +2     
  Branches       13       14       +1     
==========================================
+ Hits           56       58       +2     
  Misses          4        4              
  Partials        5        5
Impacted Files Coverage Δ
dj_database_url.py 86.56% <100.00%> (+0.41%) :arrow_up:

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

mattseymour commented 1 year ago

Need to make a small change to this there is the potential for a bug to crop up.

Brodan commented 1 year ago

Need to make a small change to this there is the potential for a bug to crop up.

Can you elaborate? I'd be happy to open a follow-up PR if this you can explain what the issue is.