Skip to content

Refactor args parsing and config management for connect and create_pool #130

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

Merged
merged 1 commit into from
May 5, 2017

Conversation

1st1
Copy link
Member

@1st1 1st1 commented May 5, 2017

Added:

  • connection_class parameter to connect() and create_pool() to
    specify a custom Connection object.

  • server_settings parameter to connect() and create_pool() to
    set custom server connection settings.

Fixed:

  • Connection pool to always create Connections with correct
    configuration. Before, the pool could lose parameters like
    statement_cache_size.

Backwards Compatibility:

  • connect() and create_pool() no longer accept arbitrary
    keyword arguments for server settings. Use new server_settings
    argument for that.

@1st1 1st1 requested a review from elprans May 5, 2017 17:38
@@ -625,8 +627,14 @@ def create_pool(dsn=None, *,
An :exc:`~asyncpg.exceptions.InterfaceError` will be raised on any
attempted operation on a released connection.
"""
if not issubclass(connection_class, connection.Connection):
Copy link
Member

Choose a reason for hiding this comment

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

We should add Connection to __init__, so that asyncpg.Connection is a thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@1st1 1st1 force-pushed the args_rework branch 3 times, most recently from 1c8c2e0 to b4da559 Compare May 5, 2017 19:20

if not addrs:
raise ValueError(
'could not determine a list of database addresses to connect to')
Copy link
Member

Choose a reason for hiding this comment

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

s/a list of database addresses/the database address/

not all(isinstance(v, str) for v in server_settings.values())):
raise ValueError(
'server_settings is expected to be None or '
'a Mapping[str, str]')
Copy link
Member

Choose a reason for hiding this comment

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

s/Mapping/Dict/

@@ -351,6 +351,30 @@ class Error(Exception):
async with pool.acquire() as con:
await con.fetchval('SELECT 1')

async def test_pool_config_persistance(self):
Copy link
Member

Choose a reason for hiding this comment

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

"persistence"

Copy link
Member

@elprans elprans left a comment

Choose a reason for hiding this comment

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

lgtm

Added:

* `connection_class` parameter to connect() and create_pool() to
  specify a custom Connection object.

* `server_settings` parameter to connect() and create_pool() to
  set custom server connection settings.

Fixed:

* Connection pool to always create Connections with correct
  configuration.  Before, the pool could lose parameters like
  `statement_cache_size`.

Backwards Compatibility:

* connect() and create_pool() no longer accept arbitrary
  keyword arguments for server settings. Use new `server_settings`
  argument for that.
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.

2 participants