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

Make keyword arguments in Python DB API connect() function more standard #2901

Closed
monetdb-team opened this issue Nov 30, 2020 · 0 comments
Closed

Comments

@monetdb-team
Copy link

@monetdb-team monetdb-team commented Nov 30, 2020

Date: 2011-10-12 13:10:34 +0200
From: Evert Rol <<evert.astro>>
To: clients devs <>
Version: 11.5.3 (Aug2011-SP1) [obsolete]
CC: @drstmane

Last updated: 2011-10-26 13:22:01 +0200

Comment 16395

Date: 2011-10-12 13:10:34 +0200
From: Evert Rol <<evert.astro>>

User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_1) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/14.0.835.202 Safari/535.1
Build Identifier:

While not specified, most Python DB APIs use the same keywords in the connect() function. These are also suggested in footnote 1 of the Python DB API specification (PEP 249):

dsn Data source name as string
user User name as string (optional)
password Password as string (optional)
host Hostname (optional)
database Database name (optional)

Currently, the monetdb DB API uses username instead of user, and hostname instead of host.
Use of the 'user' and 'host' keywords will make the DB API more compatible with existing Python DB APIs and the suggested guideline.

Reproducible: Always

I guess it would be trivial to have both user and username, and both host and hostname allowed, for backwards compatibility.

Comment 16396

Date: 2011-10-12 13:13:19 +0200
From: @grobian

yup, agree on that. Would you mind creating a patch?

Comment 16397

Date: 2011-10-12 14:37:26 +0200
From: Evert Rol <<evert.astro>>

Created attachment 79
Suggested patch

Attached patch works for me, but has not (yet) been thoroughly tested.
(NB: I don't have access to the repo, so filenames at top of patch are incorrect; in case of blindly applying this patch ;-)

Attached file: connections.py.patch (text/plain, 1650 bytes)
Description: Suggested patch

Comment 16398

Date: 2011-10-12 14:42:01 +0200
From: @grobian

LGTM.

Do you intend to test it a bit further?

Comment 16399

Date: 2011-10-12 17:59:24 +0200
From: Evert Rol <<evert.astro>>

Not really.
It's used in a practical use case, which serves as the most important test for me.
I did test for the various variants of user/username and host/hostname (including both, to test the precedence); if wanted, I can include that test in the unit test (I think that's a single Python file, from what I could find).
Other than that, I felt the change was trivial enough to not overly bother with it (famous last words).

Comment 16400

Date: 2011-10-12 18:25:29 +0200
From: @grobian

I concur, but your comment 2 suggested you'd know how to test it better. Consider it applied.

Comment 16401

Date: 2011-10-12 18:32:39 +0200
From: @grobian

bah, forgot bugref in commit:
http://dev.monetdb.org/hg/MonetDB/rev/2dd92bb63b34

Comment 16419

Date: 2011-10-14 09:22:49 +0200
From: @drstmane

Would it be possible to also add a test for this one?

Comment 16422

Date: 2011-10-14 10:14:11 +0200
From: @drstmane

re-opened to remind us that we should consider adding a test

Comment 16439

Date: 2011-10-17 14:54:27 +0200
From: Evert Rol <<evert.astro>>

Created attachment 81
Unit test patch

Not the most pretty test, but this should test various combinations of user/username and host/hostname.
I've assumed runtests.py serves as the unit test for the Python client.

Attached file: runtests.patch (text/plain, 2875 bytes)
Description: Unit test patch

Comment 16446

Date: 2011-10-19 17:19:05 +0200
From: @sjoerdmullender

Changeset 1045c895ec9e made by Sjoerd Mullender sjoerd@acm.org in the MonetDB repo, refers to this bug.

For complete details, see http//devmonetdborg/hg/MonetDB?cmd=changeset;node=1045c895ec9e

Changeset description:

Added test for bug #2901.

Comment 16447

Date: 2011-10-19 17:20:18 +0200
From: @sjoerdmullender

I added a test.
I didn't use Evert's test since we don't actually use those unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant