Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: add wrapWithDirectory flag to files.add et al #1290

Closed
wants to merge 2 commits into from
Closed

feat: add wrapWithDirectory flag to files.add et al #1290

wants to merge 2 commits into from

Conversation

wraithgar
Copy link
Contributor

For #1178

This removes the -w flag code from the cli and implements the exact same functionality in the core files component.

There is a throw in here that is being eaten (i.e. I did not see it bubble back up when testing this locally on the cli) and I do not know how to make this work, help would be appreciated here.

The reason for the throw is that this feature assumes that you gave a path name with which to wrap in a directory.

@ghost ghost assigned wraithgar Apr 2, 2018
@ghost ghost added the status/in-progress In progress label Apr 2, 2018
daviddias
daviddias previously approved these changes Apr 2, 2018
@daviddias
Copy link
Member

@@ -245,7 +247,7 @@ exports.add = {
ipfs.files.addPullStream(options),
pull.map((file) => {
return {
Name: file.path ? file.path : file.hash,
Name: file.path, //addPullStream already turned this into a hash if it wanted to
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was already being done in addPullStream so I removed it from here, because addPullStream has a new case where it is leaving path as an empty string and wants it that way.

@wraithgar
Copy link
Contributor Author

This has been updated w/ the interface and api updates.

cb(null, {
path: file.path || b58Hash,
path: opts.wrapWithDirectory ? file.path.substring(WRAPPER.length) : (file.path || b58Hash),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I admittedly don't know 100% of what's going on here or why the go implementation behaves this way but having it return an empty path for the wrapper directory feels like an anti-pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was explained as "the wrapper is the root"

@wraithgar
Copy link
Contributor Author

ipfsd-ctl will need to be updated to the latest ipfs-api for the tests to pass.

@wraithgar
Copy link
Contributor Author

needs ipfs/js-ipfsd-ctl#227

@victorb
Copy link
Member

victorb commented Apr 27, 2018

Any progress on this? Currently needed to have CI passing normally again, as feature was added in interface-ipfs-core and now used for master branch in ipfs/js-ipfs

@ghost ghost assigned victorb Apr 27, 2018
@victorb
Copy link
Member

victorb commented Apr 27, 2018

Continuing here: #1329

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants