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

Support for explicit connection management #9 #67

Closed
wants to merge 2 commits into from

Conversation

gvbgduh
Copy link
Member

@gvbgduh gvbgduh commented Mar 12, 2019

Support for this:

connection = await database.connection() ... connection.release()

@gvbgduh
Copy link
Member Author

gvbgduh commented Mar 12, 2019

Finally cleaned some mess with repos, hope it looks good, the unittest might look a bit heavy though

@tomchristie
Copy link
Member

Neato! A couple of thoughts:

  • It'd be grand if we could have tests against the public API only, rather than testing internal bits of state. (Haven't exactly thought about how we'd want to do that, I guess it's possible we might want to expose the connected/released state somehow.)
  • I'd probably switch it so that __aexit__ calls release() instead of the other way around. That'd mirror transactions then.
  • We could also support the decorator usage style that we have for transactions. (Although I'd also be okay with that coming as a later follow-up)

@gvbgduh
Copy link
Member Author

gvbgduh commented Mar 15, 2019

Thanks!

  1. Yeah, there's something wrong using private API in unites I'll adjust this. On the other hand it might be a good indicator of some missing bits in the public one.
  2. It definitely would make more sense.
  3. Good idea, I'll see how it goes.

@gvbgduh
Copy link
Member Author

gvbgduh commented Mar 15, 2019

There're fixes the first 2 points. The unittest looks a bit better.
I'm happy with any case for the connection decorator be it continuation of this one or a separate one.

@tomchristie
Copy link
Member

Closing off a bunch of PRs as stale.
If that's not the case, then you're super welcome to resubmit this, and we can go from there.

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.

None yet

2 participants