-
Notifications
You must be signed in to change notification settings - Fork 20
feat: next level of the datastore #2
Conversation
README.md
Outdated
|
||
## License | ||
|
||
MIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The README needs a reference to https://github.com/ipfs/go-datastore and also, list out all the methods that are not implemented, if any.
flow-typed/npm/pull-stream_vx.x.x.js
Outdated
} | ||
declare module 'pull-stream/util/tester.js' { | ||
declare module.exports: $Exports<'pull-stream/util/tester'>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will every module that uses flow have to generate this? Should there be a flow-pull-stream
package instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should crate a definition and pr it to the flow-typed repo if we start using flow regularly yes, for now this is just a generated stub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we can do that right now. pull-stream is part of the entire npm ecosystem, someone else will find it useful and it will save us space and time for when we decide to use it in other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the time right now to do it, we can create an issue for it
package.json
Outdated
"description": "datastore", | ||
"main": "index.js", | ||
"name": "interface-datastore", | ||
"version": "1.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep at 0.1.0 until we have it all merged
package.json
Outdated
], | ||
"author": "Juan Benet <juan@benet.ai> (http://juan.benet.ai/)", | ||
"author": "Friedel Ziegelmayer <dignifiedquire@gmail.com>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, Juan kind of created the whole thing.
Mind adding yourself as contributor?
Note, I don't think these things should be important.
|
||
export type Order<Value> = (QueryResult<Value>, Callback<QueryResult<Value>>) => void | ||
|
||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can all of this flow stuff be declared in a separate file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not, otherwise I have to change the type imports everywhere
* - `new Key('/Comedy/MontyPython/Sketch:CheeseShop')` | ||
* - `new Key('/Comedy/MontyPython/Sketch:CheeseShop/Character:Mousebender')` | ||
* | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to go into the README
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is jsdoc, will go into the generated api docs, as we do with other api docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant, the README needs to have more info about its API :)
src/shard-readme.js
Outdated
with 'SC' being the last-to-next two characters and the 'B' at the | ||
beginning of the CIDv1 string is the multibase prefix that is not | ||
stored in the filename. | ||
` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the readme a module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it needs to be included in the code. it's written into the directories that use sharding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module still has the different implementations inside it. But you also extracted some of them, let's extract all of them, making this module as light as possible.
I extracted all backing implementations, I don't think it makes sense to put all these wrapping implementations into their own repos right now, it just makes things even more complex :( |
Ref: ipfs/js-ipfs#787 |
Understood, that is because datastore has a lot of functionality, more than just using some storage backend. It seems that to make this clear we need
|
Given that |
@dignifiedquire go for it :) |
@diasdavid extraction is done |
@diasdavid updated readme to have more examples and api docs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The README is using flow notations, need to use our standard way of presenting the different types.
Other than that. LGTM
No description provided.