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

Issue 39: add clear method #50

Closed
wants to merge 3 commits into from
Closed

Conversation

toh82
Copy link
Member

@toh82 toh82 commented Jul 14, 2015

closes #39

@toh82 toh82 self-assigned this Jul 14, 2015
@toh82 toh82 mentioned this pull request Jul 14, 2015
)

.catch(function (err) {
t.ok(err instanceof Error, 'rejects error')
Copy link
Member

Choose a reason for hiding this comment

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

This is a fourth test assertion, you only plan 3 above.

This code is likely not being run ever, so we might want to remove it.

Since this is the catch() for db.get(), which we are not testing here, we don’t have to worry about testing an error case.

If this code needs a catch() to run issue free, I’d just remove the t.ok assertion.

(note that this is the first time I’m reviewing code in this part of the Hoodie world, so maybe get a second opinion from @zoepage or @gr2m :)

@janl
Copy link
Member

janl commented Jul 14, 2015

This looks well done, thank you! :)

I left a tiny comment in the test case.

Before this can be merged, it’d be great if you could squash your commits into a single commit with a message like: feat(clear): added clear() method or something like that.

Thanks again! :)

@zoepage
Copy link
Contributor

zoepage commented Jul 14, 2015

@jan: the commits are like we do this with our other plugins / modules... it's the convention ;)

@zoepage
Copy link
Contributor

zoepage commented Jul 14, 2015

getting an error:

pouchdb-hoodie-api zoepage$ node tests
module.js:338
    throw err;
    ^
Error: Cannot find module 'pouchdb/extras/promise'
    at Function.Module._resolveFilename (module.js:336:15)
    at Function.Module._load (module.js:278:25)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/zoepage/Projekte/hood.ie/pouchdb-hoodie-api/tests/utils/wait-for.js:3:15)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)

in pouchdb-hoodie-api/tests/utils/wait-for.js:3:15 we define the path to var Promise = require('pouchdb/extras/promise'), but there seems something to be missing.

@gr2m
Copy link
Member

gr2m commented Jul 14, 2015

w00p, congrats to first Hoodie pull request! 🎉 welcome to the family :)

Your test is not good yet though, I'll comment on the changes in a bit, but here's what you want:

test('removes existing db', function (t) {
  t.plan(2)

  var db = dbFactory()
  var store = db.hoodieApi()

  var localObj1 = {
    _id: 'cleanTest',
    cnt: 'cleanTest dummy doc'
  }
  store.on('clear', function () {
    t.pass("store 'clear' event was triggered")
  })

  store.add(localObj1)

  .then(function () {
    return store.clear()
  })

  .then(function () {
    return db.get('cleanTest')
  })

  .catch(function (error) {
    t.equal(error.status, 404, 'doc removed after store.clear')
  })
})

Sorry I've 3h of meetings ahead of me now, let me know if anything doesn't make sense

@toh82
Copy link
Member Author

toh82 commented Jul 14, 2015

@gr2m makes sense so i updated the PR \o/
thy for checking @janl @zoepage i'm very happy/proud to help in this project

@gr2m
Copy link
Member

gr2m commented Jul 14, 2015

all right, this looks great! Now is the time to clean up commit messages, so we can merge it :) And I know this is very confusing with all the commit conventions at all, but we are working on making them more clear and document them on hood.ie.

But anyway, can you squash these commits into these 3?

  1. test(clear): initial version
  2. feat(clear): initial version
  3. docs(README): clear method and event

Does that all make sense? I can also do the squashing for you, just let me know :)

@toh82 toh82 force-pushed the issue-39-add-clear-method branch from 4ed9dcb to d4663e9 Compare July 18, 2015 08:57
@toh82 toh82 force-pushed the issue-39-add-clear-method branch from d4663e9 to 3754bd1 Compare July 18, 2015 09:09
@gr2m
Copy link
Member

gr2m commented Jul 18, 2015

merged via c7de9ea

@gr2m gr2m closed this Jul 18, 2015
@gr2m gr2m deleted the issue-39-add-clear-method branch July 18, 2015 13:08
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.

add .clear() method
4 participants