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

Move the sqlite database module into core #671 #679

Merged
merged 15 commits into from
Oct 7, 2018

Conversation

vshelke
Copy link
Contributor

@vshelke vshelke commented Oct 1, 2018

Description

Includes moving of sqlite database module into the core of opsdroid project.

Fixes #671

Status

READY

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update
  • Documentation (fix or adds documentation)

How Has This Been Tested?

  • sqlite connection test
  • sqlite put and get test

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Member

@FabioRosado FabioRosado left a comment

Choose a reason for hiding this comment

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

Thank you so much for working on this. I updated your branch in order to get the tests to run and see if everything is okay - I will have further feedback once the tests are completed.

But I got a few suggestions in regards to your work.

Note: You might need to update your local branch before you are able to push changes back into this PR.

opsdroid/database/sqlite/__init__.py Outdated Show resolved Hide resolved
opsdroid/database/sqlite/__init__.py Outdated Show resolved Hide resolved
@vshelke
Copy link
Contributor Author

vshelke commented Oct 1, 2018

Hi @FabioRosado all tests are passed in my local for the given updated PR. But, in Travis it gives sqlite3.OperationalError: unable to open database file.
Do appdirs work differently in Travis environment ?

Copy link
Member

@FabioRosado FabioRosado left a comment

Choose a reason for hiding this comment

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

Thank you so much for these changes, things are looking great!

Unfortunately travis is complaining as the tests failed, can you have a look and try to fix them?
Coverage will also complain as it will drop the overall coverage to 90%, these are the lines that are not being covered by the tests:

opsdroid/database/sqlite/__init__.py      58     22    62%   61-66, 80-86, 101-110, 151-153, 182-184

@jacobtomlinson
Copy link
Member

I don't think appdirs should behave differently. Perhaps this is because the database exists locally for you but not in Travis?

@vshelke
Copy link
Contributor Author

vshelke commented Oct 2, 2018

Yes @jacobtomlinson .
So i checked by deleting my sqlite.db file locally the test environment makes a new sqlite.db and runs the tests successfully.
I am wondering why travis does not create the sqlite.db file in user_data_dir on running the tests ?

@jacobtomlinson
Copy link
Member

Perhaps the user_data_dir also doesn't exist on Travis?

@vshelke
Copy link
Contributor Author

vshelke commented Oct 2, 2018

So how should we get the travis build passed without the user_data_dir ?
Do i have to keep the sqlite.db file in current directory at the time of build ?

@jacobtomlinson
Copy link
Member

I think you just need to create it as part of the test setup.

@vshelke vshelke changed the title Move the sqlite database module into core #671 [WIP] Move the sqlite database module into core #671 Oct 4, 2018
@codecov
Copy link

codecov bot commented Oct 4, 2018

Codecov Report

Merging #679 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #679   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          23     24    +1     
  Lines        1460   1521   +61     
=====================================
+ Hits         1460   1521   +61
Impacted Files Coverage Δ
opsdroid/database/sqlite/__init__.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16d3a9e...9d4a6aa. Read the comment docs.

@jacobtomlinson
Copy link
Member

This is looking great. There are just a few lines which aren't being tested. If you could add a few more tests to cover everything that would be great.

@vshelke vshelke changed the title [WIP] Move the sqlite database module into core #671 Move the sqlite database module into core #671 Oct 7, 2018
@vshelke
Copy link
Contributor Author

vshelke commented Oct 7, 2018

Hi @jacobtomlinson the tests are now complete.

@FabioRosado
Copy link
Member

Thank you so much for the time working on this, I will merge it now 👍
We give stickers for contributors, send your home address by DM to opsdroid twitter and we will send some to you 😄

@FabioRosado FabioRosado merged commit 1bce0cc into opsdroid:master Oct 7, 2018
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.

Move the sqlite database module into core
3 participants