Skip to content
This repository was archived by the owner on Sep 22, 2021. It is now read-only.

Improve generation of HTTP API reference #27

Merged
merged 4 commits into from
Apr 9, 2020
Merged

Improve generation of HTTP API reference #27

merged 4 commits into from
Apr 9, 2020

Conversation

hsanjuan
Copy link
Member

@hsanjuan hsanjuan commented Apr 6, 2020

  • Document how to send the multiparts for /add
  • Bring markdown geneartion in line with ipfs-docs-v2
  • Ignore client-side options
  • Set curl -X POST
  • Do not list args of type File in arguments
  • Improve some formatting

This still leaves things at 0.4.22 so that then we can get a clean
diff of changes for 0.4.23 and eventually for 0.5.0.

Fixes #25
Fixes #4

* Document how to send the multiparts for /add
* Bring markdown geneartion in line with ipfs-docs-v2
* Ignore client-side options
* Set curl -X POST
* Do not list args of type File in arguments
* Improve some formatting

This still leaves things at 0.4.22 so that then we can get a clean
diff of changes for 0.4.23 and eventually for 0.5.0.

Fixes #25
Fixes #4
@hsanjuan hsanjuan self-assigned this Apr 6, 2020
@hsanjuan hsanjuan requested review from Stebalien and hugomrdias April 6, 2020 21:06
markdown.go Outdated
internally for some reason. To know that reason, you have to look at the
"application layer" error (usually returned with the body of the command). In
the case of streaming endpoints, they always return 200 before starting to
stream the response. Any errors are included as Trailer response headers.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we return an error before we start streaming, I think we may return a 500.

It's simpler to just say that 4xx are transport-level errors. For application level errors, you may either receive a 500 error code if the error is detected early, or a 200 response plus a trailing error if it's detected late.

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to avoid talking about transport layer vs application layer as that may not be understandable for some users. I gave it another try and tried to explain this case.

@hugomrdias
Copy link

Looks good!
Two notes probably for future work:

  • would it be possible to generate the API docs using the standard https://swagger.io/specification/ ?
  • trailer headers either need to be swapped for another solutions or at least have a secondary way to get those if the client doesnt support it

@hsanjuan
Copy link
Member Author

hsanjuan commented Apr 7, 2020

would it be possible to generate the API docs using the standard https://swagger.io/specification/ ?

Last time I checked (3 years ago), Swagger or apiary did not play well with our API. We have endpoints that support streaming and not streaming, we have multiparts, we have things that take protobufs as input, we have things that take json. Things are just not overly like the writers if RESTful API specs assume they are.

The effort to just write an accurate swagger spec for this API is gigantic, only to maybe find that at the end it cannot produce accurate docs. Additionally, the effort to maintain a swagger spec and keep it aligned is also gigantic.

Therefore, given that we have a one-click way of producing docs out of the actual code, and that this has saved us many developer hours and is mostly accurate, I take every chance to repeat that I am against the swagger approach for impractical, even if the current spec version would somehow be proven to accommodate all our API choices. I wish things were different though.

trailer headers either need to be swapped for another solutions or at least have a secondary way to get those if the client doesnt support it

Yeah!

@hsanjuan
Copy link
Member Author

hsanjuan commented Apr 9, 2020

Moving forward with this, can address anything else later.

@hsanjuan hsanjuan merged commit 5a4f2a3 into master Apr 9, 2020
@hsanjuan hsanjuan deleted the fix/improvs branch April 9, 2020 13:32
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.

Remove files from "arguments" as they aren't query parameters IPFS /api/v0/add
3 participants