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

Connection persistence in _query function #27

Closed
CeramicDingo opened this issue Jul 2, 2017 · 4 comments
Closed

Connection persistence in _query function #27

CeramicDingo opened this issue Jul 2, 2017 · 4 comments
Assignees
Milestone

Comments

@CeramicDingo
Copy link

Hi, forgive me if I'm getting this completely wrong, since I'm a novice programmer.

Within the _query function, a check is performed to see if there is a connection object available. If there isn't a new one is created.

if conn is None: if self.conn is None: conn = connection.Connection() else: conn = self.conn

The comments in the code state

If not provided, opens a new connection for this and all
subsequent queries.

However from looking at this code, it seems to me that the connection object would not be re-used for future queries, since it is being assigned to a local reference 'conn' which will be destroyed when the function exits. Therefore each time the function is called a new connection object is created?

Wouldn't the code be something like:
if conn is None: if self.conn is None: self.conn = connection.Connection() else: conn = self.conn

@veox veox self-assigned this Jul 2, 2017
@veox veox added the bug label Jul 2, 2017
@veox veox added this to the v1.0 milestone Jul 2, 2017
@veox
Copy link
Owner

veox commented Jul 2, 2017

You might have just found the biggest bug so far. :)


EDIT: Or was that keeping the keyfile resource open?..

@veox
Copy link
Owner

veox commented Jul 2, 2017

Blame points to commit af23823, from back when there was much less documentation, and when _query() wasn't really responsible for this.


The documentation that reads as currently was added this year, and (luckily) isn't present in any release: see _query() in v1.4.0.

@veox veox added the doc label Jul 2, 2017
@veox veox closed this as completed in 63ef116 Jul 2, 2017
@veox
Copy link
Owner

veox commented Jul 2, 2017

Fixed (I hope) to do in-code as it says in-docs.

Thanks for taking a look!


EDIT: Touched docs, too: https://python3-krakenex.readthedocs.io/en/latest/#krakenex.API._query

@CeramicDingo
Copy link
Author

No worries, glad I could help! 👍

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

No branches or pull requests

2 participants