Skip to content

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