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

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

Merged
merged 9 commits into from
Dec 12, 2022

Conversation

Brodan
Copy link
Contributor

@Brodan Brodan commented Jan 31, 2019

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

Fixes #102.

@Brodan
Copy link
Contributor Author

Brodan commented Feb 27, 2019

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

@sigmavirus24
Copy link
Contributor

I no longer maintain this package

@Brodan
Copy link
Contributor Author

Brodan commented Feb 28, 2019

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

@sigmavirus24
Copy link
Contributor

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

@Brodan
Copy link
Contributor Author

Brodan commented Feb 28, 2019

@kenneth-reitz Can you assist?

@Brodan
Copy link
Contributor Author

Brodan commented Apr 1, 2019

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

Some feedback on this PR your test={} 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
Copy link
Contributor Author

Brodan commented Jul 22, 2019

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

@jacobian
Copy link
Collaborator

@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
Copy link
Contributor Author

Brodan commented Jul 23, 2019

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
Copy link
Collaborator

@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
Copy link
Contributor Author

Brodan commented Jul 25, 2019

@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
Copy link
Contributor

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

@Brodan
Copy link
Contributor Author

Brodan commented Oct 7, 2019

bump!

@Brodan
Copy link
Contributor Author

Brodan commented Feb 4, 2020

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
Copy link
Contributor

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

Brodan commented Apr 28, 2020

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

Brodan commented Oct 1, 2020

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

@Brodan
Copy link
Contributor Author

Brodan commented Nov 8, 2021

@jacobian bumping again

@Brodan
Copy link
Contributor Author

Brodan commented Apr 8, 2022

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

@palfrey
Copy link
Member

palfrey commented Dec 11, 2022

@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
Copy link
Contributor Author

Brodan commented Dec 12, 2022

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

@codecov
Copy link

codecov bot commented Dec 12, 2022

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%) ⬆️

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

@palfrey palfrey merged commit d2e4719 into jazzband:master Dec 12, 2022
@mattseymour
Copy link
Contributor

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

@Brodan
Copy link
Contributor Author

Brodan commented Dec 13, 2022

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.

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.

How can we specify the name of the test database?
6 participants