search

hoodiehq / hoodie-account-client

:dog: Account client API for the browser
https://hoodiehq.github.io/hoodie-account-client
Apache License 2.0
11 stars 24 forks source link

signOut() resolves with account properties #51

Closed mxstbr closed 8 years ago

mxstbr commented 8 years ago

Closes #50

Good starter issue, because I initially had a totally different solution which was way more complex, and after I dug into the internals (get.js) a bit I realised that the fix was to simply remove the , 'account'. :+1:

gr2m commented 8 years ago

ahh I know what happened there, we changed the way account / session is stored internally. Could you uncomment this failing test and set t.plan(10) to t.plan(11)? Without your fix, the test should fail, with your fix, they should be green.

Then it would be nice if you could commit the changes in two separate commits:

  1. test: signOut() resolves with account properties
  2. fix: signOut() resolves with account properties

We follow these conventions as our release process is automated with semantic-release.

Let me know if you have any questions on how to fix your commit, or I can also do it for you :)

Guria commented 8 years ago

@gr2m very strange. why separating commits? I am sure that test updates within feat commit is fully acceptable with semantic-release processes since they are covers this concrete feature.

gr2m commented 8 years ago

yes that’s true, but you can check out the test: commit and make sure that tests failed at this point. The best way is to have separate commits, then push only the test: .. commit to the PR, and then the feat: .. or fix: .. commit, so that you can see right away that tests were failing, as CI tests get run on both

Guria commented 8 years ago

Got a point, but tests for this pr was ran only once on latest commit anyway.

gr2m commented 8 years ago

yep, but I can still go in and git checkout 22a6da200edcdb5bb8bdb8fa4eb4e45280b5d8b4 && npm test which I did as part of the PR review :)