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

API documentation improvements #3365

Merged
merged 9 commits into from Apr 6, 2023

Conversation

dbutenhof
Copy link
Member

@dbutenhof dbutenhof commented Mar 29, 2023

PBENCH-829

Fill out and update API documentation.

@dbutenhof dbutenhof added Documentation Server API Of and relating to application programming interfaces to services and functions labels Mar 29, 2023
@dbutenhof dbutenhof self-assigned this Mar 29, 2023
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

First installment....

docs/API/V1/delete.md Outdated Show resolved Hide resolved
docs/API/V1/detail.md Outdated Show resolved Hide resolved
docs/API/V1/detail.md Outdated Show resolved Hide resolved
docs/API/V1/detail.md Outdated Show resolved Hide resolved
docs/API/V1/endpoints.md Outdated Show resolved Hide resolved
docs/API/V1/endpoints.md Outdated Show resolved Hide resolved
docs/API/V1/endpoints.md Outdated Show resolved Hide resolved
@dbutenhof dbutenhof force-pushed the apidoc branch 2 times, most recently from 1742d62 to cbe04c7 Compare April 3, 2023 11:46
@dbutenhof dbutenhof requested a review from webbnh April 3, 2023 14:44
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

That's great stuff, it just needs a round of polishing. And, there is a question or two which is more to do with the design/implementation than the documentation, but the two should match....

docs/API/V1/endpoints.md Outdated Show resolved Hide resolved
docs/API/V1/endpoints.md Outdated Show resolved Hide resolved
docs/API/V1/endpoints.md Outdated Show resolved Hide resolved
docs/API/V1/endpoints.md Outdated Show resolved Hide resolved
docs/API/V1/endpoints.md Outdated Show resolved Hide resolved
docs/API/V1/upload.md Outdated Show resolved Hide resolved
docs/API/V1/upload.md Outdated Show resolved Hide resolved
docs/API/V1/upload.md Outdated Show resolved Hide resolved
docs/API/V1/upload.md Outdated Show resolved Hide resolved
docs/API/V1/upload.md Show resolved Hide resolved
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Very nice improvements!

However, my comments on docs/API/V1/update.md from my previous pass (1, 2, 3, 4) seem to have escaped your attention, and I have a couple of new comments and a couple of questions.

docs/API/V1/endpoints.md Outdated Show resolved Hide resolved
dashboard/src/utils/helper.js Outdated Show resolved Hide resolved
docs/API/V1/endpoints.md Show resolved Hide resolved
docs/API/V1/list.md Show resolved Hide resolved
docs/API/V1/list.md Outdated Show resolved Hide resolved
docs/API/V1/server_audit.md Outdated Show resolved Hide resolved
docs/API/V1/upload.md Outdated Show resolved Hide resolved
docs/API/V1/upload.md Outdated Show resolved Hide resolved
docs/API/V1/endpoints.md Outdated Show resolved Hide resolved
@webbnh
Copy link
Member

webbnh commented Apr 4, 2023

Um, Dave? Your documentation improvements failed the unit tests.... 😬

@webbnh
Copy link
Member

webbnh commented Apr 4, 2023

It would appear that Database.db_session is None. How in the ORM could that happen?

@webbnh
Copy link
Member

webbnh commented Apr 4, 2023

@npalaska...it appears you touched the code last.... 👁️

Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

You've resolved all but one of my previous comments, but...with new updates come new comments....

There's a misspelling which should be fixed, and I'd like to see more rework around the removal of "user" as a resource; other than those, I've got a couple of questions, and the rest are lesser suggestions or nits.

docs/API/V1/README.md Outdated Show resolved Hide resolved
docs/API/V1/daterange.md Show resolved Hide resolved
docs/API/V1/list.md Outdated Show resolved Hide resolved
docs/API/V1/upload.md Outdated Show resolved Hide resolved
docs/API/access_model.md Outdated Show resolved Hide resolved
docs/API/access_model.md Outdated Show resolved Hide resolved
docs/API/metadata.md Outdated Show resolved Hide resolved
docs/API/metadata.md Outdated Show resolved Hide resolved
Copy link
Member Author

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

You've resolved all but #3365 (comment) of my previous comments

I truly loathe GitHub for reviews. I made an attempt to trace all the open conversations, but GitHub makes that at least 1000 times as difficult as it should be, and it's really easy to miss things. 😦

docs/API/V1/endpoints.md Outdated Show resolved Hide resolved
docs/API/V1/server_audit.md Show resolved Hide resolved
docs/API/V1/endpoints.md Outdated Show resolved Hide resolved
docs/API/V1/upload.md Outdated Show resolved Hide resolved
docs/API/V1/daterange.md Show resolved Hide resolved
docs/API/V1/list.md Outdated Show resolved Hide resolved
docs/API/metadata.md Outdated Show resolved Hide resolved
PBENCH-829

Fill out and (to some extent update) API documentation. I debated removing
the "user" APIs now even though they still technically exist, and perhaps I
should ...
@webbnh
Copy link
Member

webbnh commented Apr 5, 2023

As long as we're griping about GitHub's review support, I'll note that some of the replies that you posted an hour ago were composed yesterday afternoon...although that might not be entirely GitHub's fault. 😉

Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Looks good to me...we just need a Jira ticket so we don't forget about reconciling the filtering options for /datasets/daterange with the analogous support for /datasets.

(Although, now that I look at it, /datasets/daterange doesn't look properly resource-centric...shouldn't daterange be a query parameter?...making that change might result in the convergence of the filtering options....)

@dbutenhof
Copy link
Member Author

Looks good to me...we just need a Jira ticket so we don't forget about reconciling the filtering options for /datasets/daterange with the analogous support for /datasets.

(Although, now that I look at it, /datasets/daterange doesn't look properly resource-centric...shouldn't daterange be a query parameter?...making that change might result in the convergence of the filtering options....)

PBENCH-1125, by the way, which I've now coded and tested; and will post as a PR once this PR goes in so I can first modify the documentation to match. 😁

And BTW I loved the suggestion of merging this with GET /datasets?daterange so that's what I did.

@dbutenhof dbutenhof merged commit 82cc01b into distributed-system-analysis:main Apr 6, 2023
4 checks passed
@dbutenhof dbutenhof deleted the apidoc branch April 6, 2023 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Of and relating to application programming interfaces to services and functions Documentation Server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants