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.

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

Document API & architecture #143

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jannis-baum
Copy link
Owner

Close #119

@jannis-baum
Copy link
Owner Author

Hey @Tweekism! I wrote the full-fledged API documentation in docs/api.md and would be happy about a review on that.

Additionally, I made a reasonably ugly draft for the documentation of the architecture of Vivify in docs/architecture.md based on your suggestion the other day that you could write that one. That's all the stuff I was thinking might make sense to document to give people an easier start into contributing to Vivify. Feel free to edit/rewrite that as much as you want, remove parts you think are obvious, and/or ask me if there is something missing/unclear :) Or also, if you think this is not necessary at all and/or you don't want to write it. No worries at all!

Thank you!!


When this is done I would ask you @tuurep to do the final review :)

@jannis-baum
Copy link
Owner Author

Ah, one more thing: This is for the state after #117 is merged

@Tweekism
Copy link
Collaborator

Cool, lemme have a look.

@Tweekism
Copy link
Collaborator

The API stuff looks really good! I think that is a big help to anyone wanting to use the API. There is a couple things I could point at if you like, I'll do that as a review thing, but you could publish it as is honestly.

I'll start working on the architecture one as soon as I can. I'll go through it slowly and make sure I understand it as I add to it.

Copy link
Collaborator

@Tweekism Tweekism left a comment

Choose a reason for hiding this comment

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

I made some notes.

I'll take the bigger architecture.md edit offline.

docs/api.md Show resolved Hide resolved

## Path encoding

Vivify's endpoints are often accessed with the pattern `/<endpoint>/<path>`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be fair to say they are always accessed with that pattern? I would consider just dropping the word often.

Copy link
Owner Author

Choose a reason for hiding this comment

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

We do have GET /health and DELETE /viewer at the root of the endpoints with no paths.

We could instead say something like

Current wording

Vivify's endpoints are often accessed with the pattern /<endpoint>/<path> where path is a percent-encoded path in the file system.

Suggestion

When accessing an endpoint corresponding to a path in the file system, use /<endpoint>/<path> where <path> is percent-encoded.

Is that better? ^^

docs/api.md Outdated Show resolved Hide resolved
docs/api.md Outdated Show resolved Hide resolved
docs/api.md Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is a rough draft, probably makes more sense if I edit it offline using my local text editor and this awesome markdown viewer I found called Vivify 🤣

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, sounds good! That's the idea that you write the whole thing haha, the stuff I wrote into the document is only for you, not for main :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rgr. Do you plan to merge api.md and leave architecture.md out? I've already pulled it down so I can keep working on it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah, I don't mind. I was thinking to leave this PR laying around until you are done with the architecture document, I don't think we are in a big rush here. Or what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fine by me, I am... thinking about putting in a job app this week, and then its my weekend for kid wrangling duties, so I might not get to it soon is all.

@jannis-baum jannis-baum marked this pull request as draft August 1, 2024 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document API & architecture
2 participants