{"action":"created","issue":{"url":"https://api.github.com/repos/dattnetwork/datt-node/issues/17","labels_url":"https://api.github.com/repos/dattnetwork/datt-node/issues/17/labels{/name}","comments_url":"https://api.github.com/repos/dattnetwork/datt-node/issues/17/comments","events_url":"https://api.github.com/repos/dattnetwork/datt-node/issues/17/events","html_url":"https://github.com/dattnetwork/datt-node/pull/17","id":103663974,"number":17,"title":"Added isomorphic ContentStore and Content type with tests","user":{"login":"willy-b","id":6969859,"avatar_url":"https://avatars.githubusercontent.com/u/6969859?v=3","gravatar_id":"","url":"https://api.github.com/users/willy-b","html_url":"https://github.com/willy-b","followers_url":"https://api.github.com/users/willy-b/followers","following_url":"https://api.github.com/users/willy-b/following{/other_user}","gists_url":"https://api.github.com/users/willy-b/gists{/gist_id}","starred_url":"https://api.github.com/users/willy-b/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/willy-b/subscriptions","organizations_url":"https://api.github.com/users/willy-b/orgs","repos_url":"https://api.github.com/users/willy-b/repos","events_url":"https://api.github.com/users/willy-b/events{/privacy}","received_events_url":"https://api.github.com/users/willy-b/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/dattnetwork/datt-node/labels/in%20progress","name":"in progress","color":"ededed"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-08-28T07:37:25Z","updated_at":"2015-08-29T03:58:58Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/dattnetwork/datt-node/pulls/17","html_url":"https://github.com/dattnetwork/datt-node/pull/17","diff_url":"https://github.com/dattnetwork/datt-node/pull/17.diff","patch_url":"https://github.com/dattnetwork/datt-node/pull/17.patch"},"body":"### ContentStore\r\nAdds a ContentStore which supports indexing by field (necessary for looking up content by user, pubkey, etc) but works isomorphically in browser and nodejs. \r\n\r\n### Content \r\nA Content type is defined in lib/content.js and has error checking, signature verification, etc supported via helper methods. \r\nAlmost all crypto uses Ryan's AsyncCrypto module and is q-promise-ified. Almost everything is tested\r\n\r\n### Tests\r\nAdded a lot of tests, mostly around all the functionality in the Content type, but in general.\r\n55 tests now"},"comment":{"url":"https://api.github.com/repos/dattnetwork/datt-node/issues/comments/135937544","html_url":"https://github.com/dattnetwork/datt-node/pull/17#issuecomment-135937544","issue_url":"https://api.github.com/repos/dattnetwork/datt-node/issues/17","id":135937544,"user":{"login":"ryanxcharles","id":63740,"avatar_url":"https://avatars.githubusercontent.com/u/63740?v=3","gravatar_id":"","url":"https://api.github.com/users/ryanxcharles","html_url":"https://github.com/ryanxcharles","followers_url":"https://api.github.com/users/ryanxcharles/followers","following_url":"https://api.github.com/users/ryanxcharles/following{/other_user}","gists_url":"https://api.github.com/users/ryanxcharles/gists{/gist_id}","starred_url":"https://api.github.com/users/ryanxcharles/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ryanxcharles/subscriptions","organizations_url":"https://api.github.com/users/ryanxcharles/orgs","repos_url":"https://api.github.com/users/ryanxcharles/repos","events_url":"https://api.github.com/users/ryanxcharles/events{/privacy}","received_events_url":"https://api.github.com/users/ryanxcharles/received_events","type":"User","site_admin":false},"created_at":"2015-08-29T03:58:58Z","updated_at":"2015-08-29T03:58:58Z","body":"Great work. Notes (from before your last two commits):\r\n\r\n- Thank you for writing lots of tests!\r\n\r\n- When using .bind on a function, you actually don't need to wrap the function in () ... seems slightly clearer to me not to. Then again, it's kind of irrelevant.\r\n\r\n- I'm not a huge fan of \"util\" objects as those problems are usually better solved by better architecture. But that's also not a big deal.\r\n\r\n- Your setOwnerPubkey should use AsyncCrypto get the address from a public key, otherwise you're doing crypto (hash function) in the main thread\r\n\r\n- setOwnerAddress has the same problem. Unfortunately converting an address to a string representation involves a hash function (base 58 check encoding).\r\n\r\n- verifySignature should (of course) use AsyncCrypto, as signiture verification/creation is one of the slowest operations we will encounter, aside from hashing a giant piece of content.\r\n\r\n- You don't need to call done() in the mocha tests. You can just return a promise. mocha understands promises and will consider the test finished when it resolves or rejects.\r\n\r\n- There shouldn't be multiple lines of indentation with promises, which in my mind ends up defeating one of the great benefits of using promises, which is ending callback pyramids. Always return a promise and use the same chain.\r\n\r\n- Your commit messages are all really long one-liners. Why not use a brief title and then a long message if necessary, as all other git-based projects do? Title on first line ( <50 chars), then if you need more, a blank line, then a message where each line is limited to 80 chars.\r\n\r\n- I get two test failures in node: @verifySignature and ContentStore \"before all\"\r\n\r\n- The karma browser tests didn't work for me at first, but then worked on subsequent tries. No idea why. Maybe this is the same problem Johan was having. If karma keeps having issues we can always get rid of it and just use the mocha browser tests.\r\n\r\n- The mocha browser tests have one test failure: @verifySignature\r\n\r\n- There shouldn't be a bunch of console.logs which show up in the tests. The only output of the tests should be whether each test passed or failed. I know you already mentioned the way to handle this is with a better logging system.\r\n"}}