-
Notifications
You must be signed in to change notification settings - Fork 423
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
docs(kubo-rps): describe JSONL returned by /api/vo/dag/import #1981
Conversation
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
Thanks for the PR @smoyer64. I can confirm that two separate JSON objects are returned, in what seems to be like Either way, this can't be merged, because these docs are actually autogenerated: ipfs-docs/docs/reference/kubo/rpc.md Lines 50 to 53 in ff99b4d
@lidel I haven't been able to figure out exactly how the docs are generated, but it seems it's a combination of: |
Well that was an epic fail ... I searched for the title line for that command and never scrolled up to see I was about to fail :) I'll amend this PR (or replace it) once I figure out how the code generation works. I believe this file was generated by the tool(s) at the first link you referenced ... and it appears to be keyed off this output type: https://github.com/ipfs/kubo/blob/f41b190e1b526ea2baa37f1ab991b7eba7b48790/core/commands/dag/dag.go#L205 That type is defined as https://github.com/ipfs/kubo/blob/f41b190e1b526ea2baa37f1ab991b7eba7b48790/core/commands/dag/dag.go#L63-L66 Which makes me think that
might be very prescient. I ran the equivalent command using the IPFS CLI and captured the response packet using WireShark. The response is two chunks each with a complete JSON document: I'll wait for @lidel to respond so I don't waste time chasing this. P.S. I asked my team members and they're vaguely remembering other places where the response is JSONL - so far, we haven't found those examples (yet). |
This code also makes me think this behavior is intentional. And I therefore think that the problem is that the |
@smoyer64 your analysis is correct, I've added more context in #1980 (comment). I'm closing this PR because the fix needs to happen elsewhere. |
Describe your changes
As described in #1980, the JSON body for the
/api/v0/dag/import
command is actually JSONL is thestats=true
argument is included.Files changed
What issue(s) does this address?
/api/v0/dag/import
is incorrect #1980Does this update depend on any other PRs?
No
Checklist before requesting a review
Checklist before merging