Dbapi2 - Githubissues

tarantool / tarantool-python

Python client library for Tarantool

https://www.tarantool.io

BSD 2-Clause "Simplified" License

100 stars 48 forks source link

Dbapi2 #161

Closed artembo closed 3 years ago

artembo commented 4 years ago

Added dbapi module accomplished according to pep-249 specification

Totktonada commented 4 years ago

It looks as the interesting feature. Thank you!

After the first very brief glance:

Please, keep Python 2 supported: CI should work for it. You can either support Python 2 in the database API implementation (it is preferred option) or disable the corresponding tests on Python 2.
Please, keep the history flat: rebase the patchset to get rid of the merge commit. (Don't hesitate to force-push your branch, it is fine for a short-term feature/bugfix branch.)
It would be good to have at least some basic test cases.
I would mention the relevant issue (#159) in the commit message, which resolves it and in the PR description.
Please, add a commit to remove Ubuntu Disco, it is EOL and fails in CI (within the patchset or in a separate PR, as you wish).

artembo commented 4 years ago

It would be good to have at least some basic test cases.

@Totktonada Many thanks for the review! Could you please clarify what kind of tests do you mean? I used dbapi-compliance package which contains all the unit tests according to pep-249 specification.

Totktonada commented 4 years ago

Please, keep the history flat: rebase the patchset to get rid of the merge commit. (Don't hesitate to force-push your branch, it is fine for a short-term feature/bugfix branch.)

I didn't meant squashing all changes into one commit, but get rid of merge commits. It is better to keep commits atomic.

It would be good to have at least some basic test cases.

<...> Could you please clarify what kind of tests do you mean? I used dbapi-compliance package which contains all the unit tests according to pep-249 specification.

My bad, I missed the inheritance.

Totktonada commented 4 years ago

I didn't meant squashing all changes into one commit, but get rid of merge commits. It is better to keep commits atomic.

That's my bad too. I see 'Dbapi2 (#160)' commit and had thought that it is the merge commit. Now I see that it is just usual commit.

Totktonada commented 4 years ago

Pushed the .gitignore update (e49f5f05db50821a17a5cfadfda521bf3c28b855) and the Ubuntu Disco removal (21e3ebf67b1be9e7009ea715afd0b419728cce98) to master.

Totktonada commented 4 years ago

Regarding 'set msgpack max version to 0.5.6 for test environment and deps' commit: I think it is quite simple to finish PR #156, this would be better than the workaround.

Totktonada commented 3 years ago

I'm glad that you found strengths to finish the work on the Database API support. It required significant effort, but you accepted the challenge and wins!