-
Notifications
You must be signed in to change notification settings - Fork 124
test(dag): ensure dag.put
can be called without options
#316
Conversation
The [dag.put](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput) interface takes an options object which is required, but should be optional with decent defaults. See dedicated test here: ipfs-inactive/interface-js-ipfs-core#316 This commit implements this behaviour. **Before**: ```js ipfs.dag.put(obj, options, (err, cid) => {...}); ``` ^ Prior to this commit, without passing `options`, this call resulted in an error. **After**: ```js ipfs.dag.put(obj, (err, cid) => {...}); ``` ^ This is now perfectly fine. Fixes ipfs#1395 License: MIT Signed-off-by: Pascal Precht <[email protected]>
The [dag.put](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput) interface takes an options object which is required, but should be optional with decent defaults. See dedicated test here: ipfs-inactive/interface-js-ipfs-core#316 This commit implements this behaviour. **Before**: ```js ipfs.dag.put(obj, options, (err, cid) => {...}); ``` ^ Prior to this commit, without passing `options`, this call resulted in an error. **After**: ```js ipfs.dag.put(obj, (err, cid) => {...}); ``` ^ This is now perfectly fine. Fixes ipfs#1395 License: MIT Signed-off-by: Pascal Precht <[email protected]>
The [dag.put](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput) interface takes an options object which is required, but should be optional with decent defaults. See dedicated test here: ipfs-inactive/interface-js-ipfs-core#316 This commit implements this behaviour. **Before**: ```js ipfs.dag.put(obj, options, (err, cid) => {...}); ``` ^ Prior to this commit, without passing `options`, this call resulted in an error. **After**: ```js ipfs.dag.put(obj, (err, cid) => {...}); ``` ^ This is now perfectly fine. Fixes ipfs#1395 License: MIT Signed-off-by: Pascal Precht <[email protected]>
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.
Sound OK to me, but I feel like missing a bit of the context that @alanshaw has, hence requesting another review from him :)
The [dag.put](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput) interface takes an options object which is required, but should be optional with decent defaults. See dedicated test here: ipfs-inactive/interface-js-ipfs-core#316 This commit implements this behaviour. **Before**: ```js ipfs.dag.put(obj, options, (err, cid) => {...}); ``` ^ Prior to this commit, without passing `options`, this call resulted in an error. **After**: ```js ipfs.dag.put(obj, (err, cid) => {...}); ``` ^ This is now perfectly fine. Fixes ipfs#1395 License: MIT Signed-off-by: Pascal Precht <[email protected]>
The [dag.put](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput) interface takes an options object which is required, but should be optional with decent defaults. See dedicated test here: ipfs-inactive/interface-js-ipfs-core#316 This commit implements this behaviour. **Before**: ```js ipfs.dag.put(obj, options, (err, cid) => {...}); ``` ^ Prior to this commit, without passing `options`, this call resulted in an error. **After**: ```js ipfs.dag.put(obj, (err, cid) => {...}); ``` ^ This is now perfectly fine. Fixes ipfs#1395 License: MIT Signed-off-by: Pascal Precht <[email protected]>
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 LGTM. Would you mind also updating the dag.put spec document to add the default values?
js/src/dag/put.js
Outdated
@@ -113,6 +113,10 @@ module.exports = (createCommon, options) => { | |||
}) | |||
}) | |||
|
|||
it('shouldn\'t fail when calling put without options', (done) => { |
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.
For consistency could you please use "should not" over "shouldn't"
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.
Done 👍
The [dag.put](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput) interface takes an options object which is required, but should be optional with decent defaults. See dedicated test here: ipfs-inactive/interface-js-ipfs-core#316 This commit implements this behaviour. **Before**: ```js ipfs.dag.put(obj, options, (err, cid) => {...}); ``` ^ Prior to this commit, without passing `options`, this call resulted in an error. **After**: ```js ipfs.dag.put(obj, (err, cid) => {...}); ``` ^ This is now perfectly fine. Fixes ipfs#1395 License: MIT Signed-off-by: Pascal Precht <[email protected]>
The [dag.put](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput) interface takes an options object which is required, but should be optional with decent defaults. See dedicated test here: ipfs-inactive/interface-js-ipfs-core#316 This commit implements this behaviour. **Before**: ```js ipfs.dag.put(obj, options, (err, cid) => {...}); ``` ^ Prior to this commit, without passing `options`, this call resulted in an error. **After**: ```js ipfs.dag.put(obj, (err, cid) => {...}); ``` ^ This is now perfectly fine. Fixes ipfs#1395 License: MIT Signed-off-by: Pascal Precht <[email protected]>
… options As discussed in ipfs/js-ipfs#1415 (comment)
License: MIT Signed-off-by: Pascal Precht <[email protected]>
@alanshaw updated the spec document. Let me know if that's how you imagined it, or if the wording needs change. |
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.
Thank you!
The [dag.put](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput) interface takes an options object which is required, but should be optional with decent defaults. See dedicated test here: ipfs-inactive/interface-js-ipfs-core#316 This commit implements this behaviour. **Before**: ```js ipfs.dag.put(obj, options, (err, cid) => {...}); ``` ^ Prior to this commit, without passing `options`, this call resulted in an error. **After**: ```js ipfs.dag.put(obj, (err, cid) => {...}); ``` ^ This is now perfectly fine. Fixes ipfs#1395 License: MIT Signed-off-by: Pascal Precht <[email protected]>
The [dag.put](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput) interface takes an options object which is required, but should be optional with decent defaults. See dedicated test here: ipfs-inactive/interface-js-ipfs-core#316 This commit implements this behaviour. **Before**: ```js ipfs.dag.put(obj, options, (err, cid) => {...}); ``` ^ Prior to this commit, without passing `options`, this call resulted in an error. **After**: ```js ipfs.dag.put(obj, (err, cid) => {...}); ``` ^ This is now perfectly fine. Fixes ipfs#1395 License: MIT Signed-off-by: Pascal Precht <[email protected]>
License: MIT Signed-off-by: Alan Shaw <[email protected]>
@PascalPrecht I added a test to ensure the defaults are set according to the spec and changed the input to the |
🎉 @alanshaw thanks! I should've added such a test in the first place.... next time! :) |
* fix(core/components/dag): make options in `put` API optional The [dag.put](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput) interface takes an options object which is required, but should be optional with decent defaults. See dedicated test here: ipfs-inactive/interface-js-ipfs-core#316 This commit implements this behaviour. **Before**: ```js ipfs.dag.put(obj, options, (err, cid) => {...}); ``` ^ Prior to this commit, without passing `options`, this call resulted in an error. **After**: ```js ipfs.dag.put(obj, (err, cid) => {...}); ``` ^ This is now perfectly fine. Fixes #1395 License: MIT Signed-off-by: Pascal Precht <[email protected]> * chore: update interface-ipfs-core dependency License: MIT Signed-off-by: Alan Shaw <[email protected]>
As discussed in ipfs/js-ipfs#1415 (comment)