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

Solving issue 84 #85

Merged
merged 3 commits into from
Jun 16, 2014
Merged

Solving issue 84 #85

merged 3 commits into from
Jun 16, 2014

Conversation

josenavas
Copy link
Contributor

Closes the connection on object deletion (I thought it was automatically closed)

@teravest can you review/merge after test pass?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) when pulling 5450049 on josenavas:issue_84 into f80166b on biocore:master.

@teravest
Copy link
Contributor

This is failing on my machine. @josenavas and I just talked about it and found a solution.

@josenavas
Copy link
Contributor Author

Ok it should be done now. The problem was that the connections created on the test's setUp function was never closed because they do not call the __del__ function explicitly. The solution is to provide a connection already when you decorate the class with qiita_test_checker and take care of its deletion explicitly there. It simplifies also test code (just a bit...)

@josenavas
Copy link
Contributor Author

@squirrelo just a heads up on this issue, so you can update your PR when this gets merged and fix your tests accordingly

@coveralls
Copy link

Coverage Status

Coverage increased (+0.12%) when pulling 585c04b on josenavas:issue_84 into f80166b on biocore:master.

@josenavas
Copy link
Contributor Author

it'd be great if this can be reviewed/merged soon!

@squirrelo
Copy link
Contributor

👍

squirrelo added a commit that referenced this pull request Jun 16, 2014
@squirrelo squirrelo merged commit 51cefe9 into qiita-spots:master Jun 16, 2014
@josenavas josenavas deleted the issue_84 branch June 21, 2014 04:10
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.

4 participants