-
Notifications
You must be signed in to change notification settings - Fork 402
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
Improve pool documentation examples #491
Conversation
I think pool documentation should recommend the safest approaches first (i.e. with the fewest possible mistakes), and discourage lower-level approach.
asyncpg/pool.py
Outdated
.. code-block:: python | ||
|
||
async with asyncpg.create_pool(user='postgres', | ||
command_timeout=60) as pool: | ||
async with pool.acquire() as con: | ||
await con.execute('CREATE TABLE names (id serial PRIMARY KEY, name VARCHAR (255) NOT NULL)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Live over 79 characters long, please fix
that's a horrible code style in your 73e717e |
@kamikaze I submitted this doc improvement a year ago. It took a month to review, after which I fixed it the same day. Now a year later you criticize it, without any constructive feedback. If the patch is wrong, reject it. But making it hang for a year without any real feedback is discouraging for the contributors. |
dont take it personal. and Im not the owner of the repo :) just dont split strings like that |
@kamikaze Please remember to be constructive and respectful in your reviews. Blank and strongly-worded characterizations of others' work are not OK. @nyurik Thank you for the contribution! Next time, please re-request a PR review, or ping in a comment whenever you make changes to address PR feedback so that it can be looked at again and not linger. |
asyncpg/pool.py
Outdated
.. code-block:: python | ||
|
||
async with asyncpg.create_pool(user='postgres', | ||
command_timeout=60) as pool: | ||
async with pool.acquire() as con: | ||
await con.execute(""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to change either docstring quotes to make this valid syntax. Also, please indent. Thanks!
@elprans thx, updated, not sure about the exact indenting -- doc-strings include all prefix spacing, making the resulting bloated with spaces... I guess it is not a big deal in this case, just something I try to avoid in general (but haven't found a perfect solution yet) |
I think pool documentation should recommend the safest approaches first (i.e. with the fewest possible mistakes), and discourage lower-level approach.