-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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(api): Improve api documentation for dashboard endpoints(filter_state, permalink, embedded) #32142
base: master
Are you sure you want to change the base?
Conversation
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.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
Category | Issue | Fix Detected |
---|---|---|
Incorrect numerical range in filter example ▹ view | ✅ | |
Unclear API Summary ▹ view |
Need a new review? Comment
/korbit-review
on this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-review
command in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-description
command in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolve
command in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Feedback and Support
superset/embedded/api.py
Outdated
@@ -71,13 +71,38 @@ def get(self, uuid: str) -> Response: | |||
"""Get the dashboard's embedded configuration. | |||
--- | |||
get: | |||
summary: Get the dashboard's embedded configuration | |||
summary: Get the dashboard's embedded configuration this endpoint is also used to embed dashboards. |
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.
Unclear API Summary ![category Documentation](https://camo.githubusercontent.com/314ccf2f17aa7961a1aa77d8ac1f7740bea9deb69b2d7498186b75c6da376ab2/68747470733a2f2f696d672e736869656c64732e696f2f62616467652f446f63756d656e746174696f6e2d376333616564)
Tell me more
What is the issue?
The summary is unclear and contains redundant information. The phrase 'this endpoint is also used to embed dashboards' is confusing and doesn't add value.
Why this matters
Unclear API documentation makes it harder for developers to understand the endpoint's primary purpose and can lead to misuse.
Suggested change ∙ Feature Preview
summary: Get configuration and renders dashboard for embedding
💬 Chat with Korbit by mentioning @korbit-ai.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #32142 +/- ##
===========================================
+ Coverage 60.48% 83.46% +22.98%
===========================================
Files 1931 545 -1386
Lines 76236 39042 -37194
Branches 8568 0 -8568
===========================================
- Hits 46114 32588 -13526
+ Misses 28017 6454 -21563
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks for the great docs @msyavuz! Can we remove the Tagging some folks that might know if that folder is needed. @Vitor-Avila @betodealmeida |
No problem! As far as i know git doesn't deal with directories so it should be out of index already since its empty? Just need to run |
@@ -1,16 +0,0 @@ | |||
# Licensed to the Apache Software Foundation (ASF) under one |
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.
Any reasons for deleting this? Can you bring it back?
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.
I think this might be used by some organizations so we don't want to delete it if it is not necessary
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.
If this folder is used, it would be good to add some comment or README file. Empty folders can be deleted at any time during cleanup efforts.
SUMMARY
Add examples to filter_state and permalink endpoints and add parameters to embedded endpoint.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After:
![image](https://private-user-images.githubusercontent.com/76224658/409968290-cb2fb25b-653d-4496-bcdc-5b8bd90267c8.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkyMjYwNTYsIm5iZiI6MTczOTIyNTc1NiwicGF0aCI6Ii83NjIyNDY1OC80MDk5NjgyOTAtY2IyZmIyNWItNjUzZC00NDk2LWJjZGMtNWI4YmQ5MDI2N2M4LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjEwVDIyMTU1NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTYzYWRiMjJhZjU0MGNhNWU1MTQ5MWExMzI5OGVhMjY4OWEyNWVlMzU2YThkYzhjNTIzOWM3ZjI1NjVjYTQ0MjgmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.aXz1LFykE7oNEVpoEFiuFLP_TqCs-HjSjRQ2G6wg7fY)
![image](https://private-user-images.githubusercontent.com/76224658/409968726-ab05e5c6-f1e7-4c24-ac70-72adbb32940c.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkyMjYwNTYsIm5iZiI6MTczOTIyNTc1NiwicGF0aCI6Ii83NjIyNDY1OC80MDk5Njg3MjYtYWIwNWU1YzYtZjFlNy00YzI0LWFjNzAtNzJhZGJiMzI5NDBjLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjEwVDIyMTU1NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTYwOTc3MjY5ZDk1MzM5ODM3MWVmYzEwN2E2MDNmMzYyZTYyYzFmZjk0MmY3OGU2Y2M5OTU2YTk5OGU0ZTdkMzgmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.fuhJks-C-KsB7MGuy02ehgIAZUYfsbUuvdOAJ8kxG-M)
![image](https://private-user-images.githubusercontent.com/76224658/409969076-c34d15f2-6e7a-49dc-a363-088a096742dd.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkyMjYwNTYsIm5iZiI6MTczOTIyNTc1NiwicGF0aCI6Ii83NjIyNDY1OC80MDk5NjkwNzYtYzM0ZDE1ZjItNmU3YS00OWRjLWEzNjMtMDg4YTA5Njc0MmRkLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjEwVDIyMTU1NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWM3NzU3OTZkNTdlZjNmYzI0Nzk5MGE1NzRkMTg5YTIyMGIwNWIwNzk5YzljODcyOTZmYTE4MDFhNTlmMzJlOTEmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.xhvsAylEA8MmX3j6sfnCvaDiY0GFmFXlKsvZ5T7g3_Q)
Note
Endpoint for filter_state accepts the body in the form of { "value": "stringified json" }. I documented the endpoint with actual json object and specified that it should be stringified and put into value field of the body in the examples. Other approach would be to just put the stringified example there which makes it unreadable but is more reflective.
TESTING INSTRUCTIONS
Go to local instance of swagger ui at /swagger/v1 and check for the documentation.
ADDITIONAL INFORMATION