-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
"pin verify". #3843
"pin verify". #3843
Conversation
This needs work, but want to ge tthe U.I down first. Here is the current usage:
|
core/commands/pin.go
Outdated
@@ -414,6 +436,90 @@ func pinLsAll(typeStr string, ctx context.Context, n *core.IpfsNode) (map[string | |||
return keys, nil | |||
} | |||
|
|||
type pinStatus struct { | |||
// use pointer to array slice to reduce memory usage |
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.
How does this reduce memory usage?
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.
Just make it a normal slice. Slices are already pointers
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.
A slice is the width of three pointers. There is a node for every indirect pin. By using an extra lawyer of indirection I reduce the size by two pointers for each node. The actual array slice will be shared many times in the common case.
00ceb4b
to
fda389b
Compare
core/commands/pin.go
Outdated
return out | ||
} | ||
|
||
func (r pinVerifyRes) Format(out io.Writer) { |
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.
Use a marshaler for this.
core/commands/pin.go
Outdated
} | ||
}() | ||
|
||
res.SetOutput(rdr) |
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.
try not to ever set the output to a reader, that makes the http api realllllly hard to deal with
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.
@whyrusleeping this is a WIP, more concerned with the U.I. will fix up this later.
5e08cbb
to
0a6d496
Compare
Here is the current usage:
Here as some sample outputs:
@whyrusleeping @Kubuxu (and others) let me know if you think the usage and output is reasonable and I will work on adding test cases and fixing the API. |
@whyrusleeping @Kubuxu (and others) let me know if you think the usage and output is reasonable and I will work on adding test cases and fixing the API. |
Is this a client side operation only, not exposed through http api? |
@hsanjuan what makes you say that? |
maybe just ignorance @kevina. Just not clear to me what the output would be in JSON since structs have private fields. |
At this point I just want to know "if you think the usage and output is reasonable". This is not ready for a full review yet. |
@kevina yeah, so this would involve knowing if the JSON output is reasonable, and how it matches the CLI-formatted output etc.. |
I was hoping for some feedback on the U.I. before I concern myself with the API and the JSON output. |
Yeah, given that the output looks nontrivial (nested things and different sorts of behaviour) it would probably be good to spec out what the api looks like here as that may be just as important as the output. That said, I think the given output looks good to me. It provides decent enough information to the end user |
@hsanjuan @whyrusleeping I fixed the API. |
@kevina thanks, can you post the json output for the examples above? |
@hsanjuan I would if I knew an easy easy way to get at it. I will hand construct it later today if necessary. |
@kevina |
Standard output:
With
With `--verbose option.
|
@kevina that api output looks good to me |
License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>
License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>
License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>
License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>
@whyrusleeping rebased |
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, thanks @kevina !
No description provided.