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

Demonstrate automatic Sphinx openapi #648

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

hmpf
Copy link
Contributor

@hmpf hmpf commented May 3, 2023

For #628

Enter docs/, run "make html", look at the result in docs/_build/html/index.html

@hmpf hmpf changed the title Deomonstrate automatic Sphinx openapi Demonstrate automatic Sphinx openapi May 3, 2023
@hmpf hmpf marked this pull request as draft May 3, 2023 06:44
@sonarcloud
Copy link

sonarcloud bot commented May 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@hmpf hmpf added documentation Improvements or additions to documentation API Affects Argus' REST API API v2 Ideas for API v2, backwards incompatible OK labels May 3, 2023
@github-actions
Copy link

github-actions bot commented May 3, 2023

Test results

       7 files     574 suites   21m 36s ⏱️
   462 tests    461 ✔️ 1 💤 0
3 234 runs  3 227 ✔️ 7 💤 0

Results for commit f9d3f37.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented May 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.61%. Comparing base (6488afa) to head (f9d3f37).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #648   +/-   ##
=======================================
  Coverage   84.61%   84.61%           
=======================================
  Files          75       75           
  Lines        3750     3750           
=======================================
  Hits         3173     3173           
  Misses        577      577           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lunkwill42 lunkwill42 added the paused Assignee is busy with things of higher priority label Jan 29, 2024
@lunkwill42 lunkwill42 linked an issue Jan 29, 2024 that may be closed by this pull request
@hmpf hmpf self-assigned this Apr 10, 2024
@hmpf
Copy link
Contributor Author

hmpf commented Apr 10, 2024

I suspect this is a bug in the sphinx plugin:

reading sources... [100%] site-specific-settings
WARNING: unknown directive or role name: django:setting
<openapi>:1: ERROR: Unexpected indentation.
/home/hm/GIT/campus/argus/Argus/docs/openapi.rst:2416: WARNING: Block quote ends without a blank line; unexpected unindent.
<openapi>:1: ERROR: Unexpected indentation.
/home/hm/GIT/campus/argus/Argus/docs/openapi.rst:2422: WARNING: Block quote ends without a blank line; unexpected unindent.

Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

I think you need to also change the .readthedocs.yaml file to make it install the added optional dependencies. I think you can use the extra_requirements option in https://docs.readthedocs.io/en/stable/config-file/v2.html#packages

Otherwise this seems to work. Even though the look of it could be improved.

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

Looks like a good start, but it really needs to explain how argus-openapi.yml is generated.

Also, some of the output looks weird with our chosen Sphinx templates, like the Status codes from this example:
image

Copy link
Contributor

@podliashanyk podliashanyk left a comment

Choose a reason for hiding this comment

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

Thats a functioning POC 👍
There are improvements that could be made in the future:

  • those mentioned above
  • contrast errors should be fixed. The WAVE accessibility tool shows whooping 500 contrast errors, but it seems that they all concern the yellow and the green text as shown below:
Screenshot 2024-04-16 at 16 16 16
  • indentations should be fixed so that alignment of the same level elements matches, and so that indentations are more uniform in general:
Screenshot 2024-04-16 at 16 11 11
  • Some indentations could probably be increased for better readability in my opinion (Parameters, Status codes etc elements)
  • Some indentations could be reduced for better readability in my opinion:
Screenshot 2024-04-16 at 16 04 59

@hmpf
Copy link
Contributor Author

hmpf commented Apr 17, 2024

Thats a functioning POC 👍 There are improvements that could be made in the future:

  • those mentioned above
  • contrast errors should be fixed. The WAVE accessibility tool shows whooping 500 contrast errors,
    but it seems that they all concern the yellow and the green text
  • indentations should be fixed so that alignment of the same level elements matches, and so that
    indentations are more uniform in general
  • Some indentations could probably be increased for better readability in my opinion
    (Parameters, Status codes etc elements)
  • Some indentations could be reduced for better readability in my opinion

I don't know how much of this is controlled by our sphinx theme or whether we'd have to fork the plugin to make the changes.

.readthedocs.yaml Outdated Show resolved Hide resolved
@hmpf
Copy link
Contributor Author

hmpf commented Apr 19, 2024

Replacing the default sphinx theme (alabaster) with "sphinx_rtd_theme" yields this:

Screenshot_20240419_083821

What do you think, @podliashanyk?

@johannaengland
Copy link
Contributor

Replacing it with "sphinx_rtd_theme" is also nice, since we use the same for NAV!

@hmpf
Copy link
Contributor Author

hmpf commented Apr 19, 2024

The API looks like barf with sphinx_rtd_theme, the indents don't make sense. I'll add some more screenshots.

@hmpf
Copy link
Contributor Author

hmpf commented Apr 19, 2024

@hmpf
Copy link
Contributor Author

hmpf commented Apr 19, 2024

Theme: "sphinx_rtd_theme"

sphinx_rtd_theme at theme gallery

Screenshot_20240419_083821

@hmpf
Copy link
Contributor Author

hmpf commented Apr 19, 2024

Theme: "sphinx_book_theme"

sphinx_book_theme at gallery

Screenshot_20240419_141343

This also has a darkmode-theme.

@hmpf
Copy link
Contributor Author

hmpf commented Apr 19, 2024

Theme: "furo"

furo on gallery

Screenshot_20240419_141843

It is possible to tune a lot with furo: https://pradyunsg.me/furo/customisation/

@hmpf
Copy link
Contributor Author

hmpf commented Apr 19, 2024

I don't think this will be ready for next release.

@hmpf hmpf added rc-future and removed paused Assignee is busy with things of higher priority labels Apr 19, 2024
Copy link

sonarcloud bot commented Apr 23, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API v2 Ideas for API v2, backwards incompatible OK API Affects Argus' REST API documentation Improvements or additions to documentation rc-future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants