API revamp and cleansing #140

pgte · 2017-06-29T08:38:31Z

Fixes #139

daviddias

left some comments. Let's not forget the repo.exists

Thanks @pgte :):)

daviddias · 2017-06-29T09:06:52Z

README.md

+  * `storageBackends` (object, optional): may contain the following values, which should each be a class implementing the [datastore interface](https://github.com/ipfs/interface-datastore#readme):
+    * `root` (defaults to [`datastore-fs`](https://github.com/ipfs/js-datastore-fs#readme) in Node.js and [`datastore-level`](https://github.com/ipfs/js-datastore-level#readme) in the browser)
+    * `blocks` (defaults to [`datastore-fs`](https://github.com/ipfs/js-datastore-fs#readme) in Node.js and [`datastore-level`](https://github.com/ipfs/js-datastore-level#readme) in the browser)
+    * `datastore` (defaults to `datastore-level`)


It would be nice to have a brief description of what the role of each of these datastores are at a high level.

@pgte, could you add this brief description?

daviddias · 2017-06-29T09:08:50Z

README.md

+### repo.close (callback)
+
+Unlocks the repo.
+


Let's break the API description into 3 sets:

Set up - init, open, close and exists

Repos - Explaining that there are two repos

Utility/Convinience - apiAddr, config, version..

daviddias · 2017-06-29T09:12:13Z

README.md

+
+### repo.config.get(key:string, callback)
+
+Get a config value. `callback` is a function with the signature: `function (err, value)`, wehre the `value` is of the same type that was set before.


Currently, the 'replace a key' is being done inside js-ipfs - https://github.com/ipfs/js-ipfs/blob/master/src/core/components/config.js#L10-L55

I like that it is being exposed here, avoids duplication of code if any other module needs to do the same. Let's make sure that once this is done we update js-ipfs to just call the methods directly

Yes, it's in https://github.com/ipfs/js-ipfs/pull/887/files#diff-10e3e7645db7852759507e460173d205

daviddias · 2017-06-29T09:13:54Z

src/api-addr.js

+
+        callback(err, value && value.toString())
+      })
+    },


since you are doing the check for value, this method can be simplified to:

get (callback) { store.get(apiFile, (err, value) => callback(err, value && value.toString())) }

daviddias · 2017-06-29T09:16:17Z

src/default-options.js

+    },
+    blocks: {
+      sharding: true
+    }


It's missing the extension .data

daviddias · 2017-06-29T09:17:22Z

src/index.js

@@ -97,60 +75,54 @@ class IpfsRepo {
   */
  open (callback) {
    if (!this.closed) {
-      return callback(new Error('repo is already open'))
+      callback(new Error('repo is already open'))
+      return // early


setImmediate the callback

daviddias · 2017-06-29T09:19:21Z

src/index.js

+        const blocksBaseStore = backends.create('blocks', path.join(this.path, 'blocks'), this.options)
+        blockstore(
+          blocksBaseStore,
+          this.datastore,


Let's reference to this.datastore as this.rootDatastore (or a name that suggests that it is the root of all mount points, maybe add this as a comment)

You mean change this.datastore to this.rootDatastore in the public API, add an alias or simply add a comment?

ah, wait.

this.datastore is /datastore? If so, why does /blocks need it?
if this.datastore is just /, then it makes sense to call it rootDatastore

Ah, it's legacy, it used to need it but now doesn't. Good catch, removing it.

daviddias · 2017-06-29T09:19:56Z

src/index.js

+      {
+        config: (cb) => this.config.exists(cb),
+        version: (cb) => this.version.check(repoVersion, cb)
+      },


Interesting, why making this an object?

Just aesthetics: I prefer to refer to result.config rather than result[0].

nice. That I didn't know :)

daviddias · 2017-07-01T09:26:16Z

README.md

+Tells whether this repo exists or not. Callsback with `(err, bool)`.
+
+### Repos
+


let's add a line here saying "root repo"

daviddias · 2017-07-01T09:27:27Z

README.md

+
+* `key` can be a buffer, a string or a [Key](https://github.com/ipfs/interface-datastore#keys).
+* `callback` is a callback function `function (err, result:Buffer)`
+


let's add a line here saying "blocks/blockstore" and link it to the ipfs-block class

daviddias

🎉🎉🎉🎉🎉🎉 Thank you @pgte, this module is getting so much better structured, really like the new way things are mounted.

Could you be extra rigorous with interop testing and by this I mean:

get the latest go-ipfs
init a clean repo
add 3 objects
add a couple of files
keep all of those hashes
bring that new repo to the test folder
Make sure you can read all that data
Do the same for some DHT records and or pinroot set hash (that gets stored in the datastore level)

daviddias · 2017-07-01T09:28:42Z

README.md

+This is contains a full implementation of [the `interface-datastore` API](https://github.com/ipfs/interface-datastore#api).
+
+
+### Convenience


Maybe call this utils? They are convenience methods but typically these are listed as utils

daviddias · 2017-07-01T09:29:38Z

README.md

+### Convenience
+
+#### repo.config
+


Explain why using repo.config is better than repo.set('config')

daviddias · 2017-07-01T09:29:48Z

README.md

+    assert.equal(config.a.b.c, 'c value')
+  })
+})
+``


daviddias · 2017-07-01T09:30:56Z

README.md

+
+Gets the API address.
+
+#### repo.apiAddr.set (value:string, callback)


Should take a String or multiaddr, but should always be a valid multiaddr

daviddias · 2017-07-01T09:36:45Z

@justinmchase I believe you are going to like the results of this PR as it will make way more obvious and testable that S3 backend you are looking for :)

victorb · 2017-07-01T10:21:13Z

README.md

+* `key` is a string specifying the object path. Example:
+
+```js
+repo.config.set('a.b.c', (err, value) => {


Should be .get

good catch, fixed

victorb · 2017-07-01T10:24:41Z

src/index.js

-      }
+function ignoringAlreadyOpened (cb) {
+  return (_err) => {
+    let err = _err


Not 100% I understand this type of assignment, why not name it directly err and just use that?

The only reason is that I think that assigning to an argument is a bad practise..

A couple things here, its fairly standard to have a code property on an error in addition to message for this sort of situation (e.g. fs.stats documentation).

Also, the parameter should be called error for convention purposes, standard will do some enforcement around variables of that name specifically as well.

I would probably write this as:

function ignoringAlreadyOpened (cb) { return (err) => err && err.code === 'AOPEN' ? callback() : callback(err) }

This would imply you are adding the code 'AOPEN' too of course. You don't want to really assume the error message is going to be English.

We currently don't have unified error handling on the datastore interfaces. These errors come directly from them which is why have to make string matching right now. I agree it would be better to have subclasses of Error + error codes for checking these but these need to be implemented first in all the datastores.

dignifiedquire · 2017-07-02T13:47:01Z

README.md

+Sets the API address.
+
+* `value` should be a [Multiaddr](https://github.com/multiformats/js-multiaddr) or a String representing a valid one.
+


seems to be missing exists

dignifiedquire · 2017-07-02T13:48:42Z

src/api-addr.js

+     * @returns {void}
+     */
+    set (value, callback) {
+      store.put(apiFile, Buffer.from(value.toString()), callback)


Buffer.from is not available in node@4 you will need to include safe-buffer

dignifiedquire · 2017-07-02T13:50:31Z

src/config.js

        } catch (err) {
          return callback(err)
        }
-        callback(null, config)
+        if (key !== undefined && !_has(config, key)) {
+          callback(new Error('Key does not exist in config'))


would be nice to say which key doesn't exist, i.e.Key "foo" does not exist in config

dignifiedquire · 2017-07-02T13:57:26Z

src/index.js

+    if (err && err.message.startsWith('ENOENT')) {
+      err = null
+    }
+    cb(err)


These to ignore calls could share some code, sth like this

const ignoreIf = (cond) => (cb) => (err) => { cb(err && !cond(err) ? err : null) } const ingoreNotFound = ignoreIf((err) => err.message.startsWith('ENOENT')) const ignoreAlreadyOpen = ignoreIf((err) => err.message === 'Already open'))

Same with this, you should probably have error codes on these.

justinmchase · 2017-07-02T21:56:50Z

src/blockstore.js

+    if (err) {
+      callback(err)
+      return // early
+    }


This section can be a single line, which standard will allow:

if (err) return callback(err)

but our linting rules do not, right @diasdavid ;)

I'm not against to if (err) { return callback(err) } if the goal is to save a couple of lines.

justinmchase · 2017-07-02T22:39:15Z

src/default-options.js

+      sharding: true,
+      extension: '.data'
+    }
+  }


justinmchase · 2017-07-02T22:54:37Z

src/index.js

-const levelDirectory = 'datastore'
+const lockers = {
+  memory: require('./lock-memory'),
+  fs: require('./lock')


Shouldn't the locks also be a datastore? What if you are running on an ephemeral server and it goes down and back up, would the locks need to be preserved or is it ok that they are lost when the server is replaced? For example if your server was a Docker container and you update the instance.

Lock can't be a simple datastore as it uses os specific system calls on the filesystem. Though the memory lock could use something else as backend if needed. It might be good to allow passing in ones own lock module if one wants to instead of using one of the specified

pgte · 2017-07-03T15:36:43Z

I think I've addressed every concern here. Please tell me if there's anything else I need to do to make this happen, as I depend on this. :)

daviddias

Only minor comments for cosmetic changes.

Have you:

tested with a fresh repo from go-ipfs 0.4.10?
tested all the dependents that live inside the IPFS org? If yes, could you list them in awesome endeavour PR

daviddias · 2017-07-03T19:11:51Z

README.md

+  * `storageBackends` (object, optional): may contain the following values, which should each be a class implementing the [datastore interface](https://github.com/ipfs/interface-datastore#readme):
+    * `root` (defaults to [`datastore-fs`](https://github.com/ipfs/js-datastore-fs#readme) in Node.js and [`datastore-level`](https://github.com/ipfs/js-datastore-level#readme) in the browser)
+    * `blocks` (defaults to [`datastore-fs`](https://github.com/ipfs/js-datastore-fs#readme) in Node.js and [`datastore-level`](https://github.com/ipfs/js-datastore-level#readme) in the browser)
+    * `datastore` (defaults to `datastore-level`)


@pgte, could you add this brief description?

daviddias · 2017-07-03T19:12:08Z

README.md

+
+#### repo.exists (callback)
+
+Tells whether this repo exists or not. Callsback with `(err, bool)`.


Missing space

daviddias · 2017-07-03T19:12:50Z

README.md

+
+Root repo:
+
+#### repo.put (key, value:Buffer, callback)


Unnecessary spaces.

Also, can we put these between ``, helps readability on the readme.

daviddias · 2017-07-03T19:13:25Z

README.md

+
+```js
+repo.config.set('a.b.c', 'c value', (err) => {
+  if (err) throw err


please use {}, even if they are all in the same line so that it follows our agreed codestyle

daviddias · 2017-07-03T19:14:20Z

README.md

+  })
+})
+```
+


daviddias · 2017-07-03T19:14:33Z

README.md

+Sets the API address.
+
+* `value` should be a [Multiaddr](https://github.com/multiformats/js-multiaddr) or a String representing a valid one.
+


pgte · 2017-07-04T15:23:02Z

@diasdavid listed dependents in awesome endeavour PR: ipfs/js-ipfs#887

pgte · 2017-07-04T15:54:10Z

@diasdavid 85a2810 and c5695ed test this against a repo generated by go-ipfs v0.4.10.

pgte added 2 commits June 28, 2017 13:13

documenting new API

872284b

cleansing and new API

929695b

Kubuxu added the status/in-progress In progress label Jun 29, 2017

pgte changed the title ~~API revamp and cleansing~~ WIP: API revamp and cleansing Jun 29, 2017

get and set specific parts of the config value

5bf8ad4

pgte changed the title ~~WIP: API revamp and cleansing~~ API revamp and cleansing Jun 29, 2017

daviddias requested changes Jun 29, 2017

View reviewed changes

pgte added 9 commits June 29, 2017 10:31

small fixes

6029487

readme: broke down API into sections

85380a5

docs: correcting the blocks API

1c877fa

fixed minor spelling mistake on error message

13499cd

documented repo.exists

210e494

blockstore does not need datastore any more

8904d7a

config.set supports object path key

7c59443

docs: corrected config api and added example

87a40b1

fail when config doesnt have the key

41b021e

This was referenced Jun 30, 2017

feat: adapted to new ipfs-repo API ipfs/js-ipfs#887

Merged

Adapted to new ipfs-repo API ipfs/js-ipfs-block-service#66

Merged

Adapted to new ipfs-repo API ipld/js-ipld#84

Merged

WIP: adapted to new ipfs-repo API ipfs/js-ipfs-bitswap#144

Merged

daviddias reviewed Jul 1, 2017

View reviewed changes

daviddias requested changes Jul 1, 2017

View reviewed changes

daviddias requested review from victorb, dignifiedquire and kumavis July 1, 2017 09:35

victorb suggested changes Jul 1, 2017

View reviewed changes

pgte added 2 commits July 1, 2017 11:32

docs: API clarifications and slight changes

424d0d8

docs: more slight readme changes

77d1e43

pgte added 3 commits July 1, 2017 11:36

docs: added link to the the ipfs-blocks repo

bb6dccd

fix: docs api and example typo

90ae830

docs: fixed typos

25bab69

dignifiedquire reviewed Jul 2, 2017

View reviewed changes

justinmchase reviewed Jul 2, 2017

View reviewed changes

src/default-options.js

sharding: true,

extension: '.data'

}

}

Copy link

Contributor

justinmchase Jul 2, 2017

Choose a reason for hiding this comment

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

👍

daviddias reacted with thumbs up emoji

justinmchase reviewed Jul 2, 2017

View reviewed changes

pgte added 3 commits July 3, 2017 12:20

using safe-buffer

1cf93ab

succumbing to one liner return callback

bf82528

DRYer and more elegant error ignoration

174fff0

daviddias requested a review from dignifiedquire July 3, 2017 19:11

daviddias requested changes Jul 3, 2017

View reviewed changes

pgte added 4 commits July 4, 2017 13:57

validating arguments and supporting get with undefined key

8e8055d

docs: some README fixes

40791c5

docs: added repo.config.exists

a156241

docs: added a bit of explanation to the meaning of each repo type

a9293e2

pgte added 2 commits July 4, 2017 16:51

tests: new test repo out of go-ipfs v0.4.10

85a2810

tests: some more interop tests for blocks and datastore

c5695ed

daviddias approved these changes Jul 4, 2017

View reviewed changes

tests: fixed missing level file

bda0e7f

daviddias merged commit 0208d60 into master Jul 4, 2017

daviddias removed the status/in-progress In progress label Jul 4, 2017

daviddias deleted the feat/api-cleansing branch July 4, 2017 18:23

daviddias mentioned this pull request Jul 10, 2017

⚡️ v0.25.0 RELEASE 🚀 ipfs/js-ipfs#910

Closed

13 tasks


		### repo.config.get(key:string, callback)

		Get a config value. `callback` is a function with the signature: `function (err, value)`, wehre the `value` is of the same type that was set before.

		Tells whether this repo exists or not. Callsback with `(err, bool)`.

		### Repos


		* `key` can be a buffer, a string or a [Key](https://github.com/ipfs/interface-datastore#keys).
		* `callback` is a callback function `function (err, result:Buffer)`

		This is contains a full implementation of [the `interface-datastore` API](https://github.com/ipfs/interface-datastore#api).


		### Convenience


		Gets the API address.

		#### repo.apiAddr.set (value:string, callback)

		Sets the API address.

		* `value` should be a [Multiaddr](https://github.com/multiformats/js-multiaddr) or a String representing a valid one.


		#### repo.exists (callback)

		Tells whether this repo exists or not. Callsback with `(err, bool)`.

API revamp and cleansing #140

API revamp and cleansing #140

Conversation

pgte commented Jun 29, 2017

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Jul 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmchase Jul 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgte commented Jul 3, 2017

daviddias left a comment

Choose a reason for hiding this comment

daviddias Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgte commented Jul 4, 2017

pgte commented Jul 4, 2017

daviddias Jul 3, 2017 •

edited

Loading

justinmchase Jul 2, 2017 •

edited

Loading

daviddias Jul 3, 2017 •

edited

Loading