Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

update link to filestore experimental status #4557

Merged
merged 1 commit into from
Feb 14, 2018
Merged

update link to filestore experimental status #4557

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/commands/add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ You can now check what blocks have been created by:
hashFunStr, hfset, _ := req.Option(hashOptionName).String()

if nocopy && !cfg.Experimental.FilestoreEnabled {
res.SetError(errors.New("filestore is not enabled, see https://git.io/vy4XN"),
res.SetError(errors.New("filestore is not enabled, see https://git.io/vNItf"),
cmdkit.ErrClient)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a personal dislike for the use of short links unless absolutely necessary and would prefer https://github.com/ipfs/go-ipfs/blob/master/docs/experimental-features.md#ipfs-filestore is used.

But since a short URL was already used I am not going to block this.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is because it happens in an error message. Tools have a bad habit of truncating logs/error messages so this is safer...

Copy link
Member

Choose a reason for hiding this comment

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

How about changing the error to print what the user has to do to enable it + the link to the doc? Something like:

Filestore is not enabled. Run `ipfs config --json Experimental.FilestoreEnabled true` to enable it. See https://git.io/vNItf for more information.

As it stands right now, unless I have internet access, it becomes hard to figure out how to enable it. And if the user is not interested in more info, just to enable the feature, this would make the flow the quickest.

Copy link
Member

Choose a reason for hiding this comment

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

I'd do that as a separate change. This fixes a specific bug.

(Really, I'd like to ship an immutable datastore with IPFS that includes the docs. Then, we could just tell people to read, e.g., /ipfs/QmId/...).

Copy link
Member

Choose a reason for hiding this comment

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

@Stebalien even with that, it's way simpler just to tell what has to be done by the user to achieve what they initially wanted to do, rather than having the indirection of going to a webpage/cat a specific file.

Copy link
Contributor

Choose a reason for hiding this comment

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

@victorbjelkholm for this particular feature there isn't anything useful written in the docs, but in general I think it is better to point people to docs when enabling an experimental feature so they know the risks (if any) invoiced rather then blindly following some instructions in an error message.

return
}
Expand Down