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

Extract ConnectionManager to handle setup connection before using it … #1457

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

anih
Copy link
Contributor

@anih anih commented Dec 29, 2016

…by Document

Refs #1446, #788, #1487

@anih
Copy link
Contributor Author

anih commented Dec 29, 2016

Closes #1446 and should fix #788

lanfon72 added a commit to lanfon72/mongoengine that referenced this pull request Jan 6, 2017
class ConnectionManager(object):
connections_registry = defaultdict(dict)

def get_and_setup(self, doc_cls, alias=None, collection_name=None):
Copy link
Member

Choose a reason for hiding this comment

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

This could use a docstring, especially as a public method. What it doesn, when it should be used, etc.


@classmethod
def _get_db(cls, alias):
"""Some Model using other db_alias"""
Copy link
Member

Choose a reason for hiding this comment

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

The docstring here is pretty confusing. What is "Some Model"? Why is it using "other db_alias"? Why do we need a private method that only proxies a call to a public get_db?

conn = get_connection(alias)
conn.drop_database(db)

def reset(self):
Copy link
Member

Choose a reason for hiding this comment

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

This could use a docstring, too. Additionally, should this method perform any cleanup of the connections before dropping references to them? Will that leave lingering unclosed connections to the database?

@wojcikstefan
Copy link
Member

Hi @anih at a glance this looks great! I haven't deeply read through the code yet, only asked for a few docstrings given that we're working with some public methods we want developers to interact with and understand. Could you add those as well as pull in the master and handle the conflicts?

screen shot 2017-01-07 at 7 08 38 pm

Absolutely <3 <3 <3 changes that remove cruft from this library AND fix issues at the same time!!! :)

@wojcikstefan
Copy link
Member

Have you had a chance to read my comments @anih?

@anih
Copy link
Contributor Author

anih commented Jan 12, 2017

Hi @wojcikstefan I had chance but hadn't time to implement them and to finish whole pull request. I will try to do that next week.

@wojcikstefan
Copy link
Member

Hi @anih have you had some time to look into this PR by any chance? :)

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