2.2.0 - Githubissues

briangonzalez commented 7 years ago

A couple things to note here:

requireSqlite() was unnecessary imo. To hand wave over this fact, I included sqlite as an actual dep and not an optional one.
I moved some of the db-specific logic into a module. This may seem unnecessary now, but I am setting myself up to make the code more organized and ultimately more testable once it comes time to write some units.
getRecentChats returns a promise which resolves to an array of chats. The default limit is 10.

wtfaremyinitials commented 7 years ago

Please make sure the new DB implementation opens chat.db as readonly. I'm not comfortable opening it as writable on the master branch due to caution of corrupting people's iMessage data.

briangonzalez commented 7 years ago

@wtfaremyinitials Let me figure out how to do that. Any other concerns?

briangonzalez commented 7 years ago

@wtfaremyinitials Good to go.

https://github.com/wtfaremyinitials/osa-imessage/pull/14/files#diff-af7ba70fb7769f61fd8354654dc56000R8

wtfaremyinitials commented 7 years ago

Finally got some time to properly review this.

Looks great, switching to async/await was a good call. In addition, now that osa-imessage has a APIs to do with reading the database, I have a few new feature ideas that I'll probably give a shot later this week.

I did notice a few issues though. Hopefully they won't be difficult to fix.

It looks like some debug code got left in index.js https://github.com/briangonzalez/osa-imessage/blob/2.2.0/index.js#L179-L182
I got this error { Error: SQLITE_MISUSE: Database handle is closed errno: 21, code: 'SQLITE_MISUSE' } with this as a simple test case
```
var im = require('./')
```

im .getRecentChats() .then(d => console.log(d)) .catch(e => console.error(e))



3. I'll admit that I'm still somewhat new to sqlite, but is it proper form to open and close the database for every function call? Might be related to point #&#xfeff;2
https://github.com/briangonzalez/osa-imessage/blob/2.2.0/index.js#L158

Thanks for your help! I can't wait to get this merged.  😄

briangonzalez commented 7 years ago

@wtfaremyinitials Good catches. I'll look shortly.

briangonzalez commented 7 years ago

I am going to cleanup the commits once you look over what I've done!

wtfaremyinitials commented 7 years ago

Looks good to me. Let me now when you're ready for me to merge it

briangonzalez commented 7 years ago

Alright, we're cleaned up.

wtfaremyinitials commented 7 years ago

🎉

wtfaremyinitials / osa-imessage

2.2.0 #14