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.

Sign up for GitHub

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

Jump to bottom

Small documentation fixes. #713

Merged
merged 6 commits into from
Apr 21, 2022
Merged

Small documentation fixes. #713

merged 6 commits into from
Apr 21, 2022

Conversation

ngnpope
Copy link
Contributor

@ngnpope ngnpope commented Apr 20, 2022

No description provided.

@tfranzel
Copy link
Owner

thx! haha I actually like my word creation there.... hyperliked 😎

@ngnpope
Copy link
Contributor Author

ngnpope commented Apr 20, 2022

If I catch you in time, I've got a few more I'll add to this!

@codecov
Copy link

codecov bot commented Apr 20, 2022 •
edited
Loading

Codecov Report

Merging #713 (0728fcb) into master (4b7eee7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #713   +/-   ##
=======================================
  Coverage   98.80%   98.80%           
=======================================
  Files          64       64           
  Lines        7290     7290           
=======================================
  Hits         7203     7203           
  Misses         87       87
Impacted Files Coverage Δ
drf_spectacular/extensions.py 100.00% <ø> (ø)
drf_spectacular/plumbing.py 97.27% <ø> (ø)
drf_spectacular/utils.py 99.55% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b7eee7...0728fcb. Read the comment docs.

@tfranzel
Copy link
Owner

should i keep it open? let me know when you are done here so I can merge.

@ngnpope
Copy link
Contributor Author

ngnpope commented Apr 20, 2022

Should be ready now.

tfranzel
tfranzel reviewed Apr 20, 2022
View reviewed changes
Copy link
Owner

@tfranzel tfranzel left a comment

Choose a reason for hiding this comment

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

woah great work!

the spec anchors still generate complaints:

( drf_spectacular: line    6) broken    https://spec.openapis.org/oas/v3.0.3#parameter-object - Anchor 'parameter-object' not found                                                                                                                                                         
( drf_spectacular: line    8) broken    https://spec.openapis.org/oas/v3.0.3#schema-object - Anchor 'schema-object' not found                                                                                                                                                               
( drf_spectacular: line   11) broken    https://spec.openapis.org/oas/v3.0.3#security-scheme-object - Anchor 'security-scheme-object' not found

was it broken before too?

as a sidenote, the changelog shell script generates these links. I simply relied on the github redirects as the numbers are unique identifiers and the script cannot easily distinguish between issues and PRs.

docs/conf.py Show resolved Hide resolved
ngnpope and others added 6 commits April 21, 2022 10:08
@ngnpope
Small documentation fixes.
dc2f343 
Fixes various typographical errors, spurious characters, etc.
@ngnpope
Avoid using default role in documentation.
f11081a 
Sphinx's default role (wrapping in single backticks) is often confusing
because it doesn't render as code - which achieved using double
backticks in reStructuredText - and this differs from Markdown. In
Sphinx, the default role is rendered in the same way as wrapping with
single asterisks by default - providing italicised emphasis.

To avoid this confusion, we can override the default role to warn us
when we are using it and change existing usages to use correct markup -
double backticks where we want code, single asterisks where we want
emphasis.
@ngnpope
Fixed broken links in documentation.
d3e8c55 
Also updated links to canonical locations to reduce redirects.

These were discovered using `make linkcheck`.

I did something similar a while back for Django:

    django/django#14325
@ngnpope
Remove invalid example in drf-yasg migration documentation.
372d195 
Status codes are only for responses!
@ngnpope
Consistent spacing and underlines for headings in documentation.
86753a9
@ngnpope
Fixed issues raised by sphinx-lint.
0728fcb
@ngnpope
Copy link
Contributor Author

ngnpope commented Apr 21, 2022

was it broken before too?

Yes. In a sense. The issue here is that the anchors on the page are dynamically generated via JavaScript. This means that the linkcheck builder is unable to find them. The original link was pointing to the Markdown file for the spec in GitHub which suffers from the same issue. This is because the anchors for the headings there are stored prefixed with user-content-, e.g. #user-content-parameter-object which is transformed to #parameter-object in JavaScript. You can add the prefix which fixes the output from the linkcheck builder, but it's not that nice. See sphinx-doc/sphinx#9016.

Regardless, I think it's better that we use the documentation hosted under spec.openapis.org. Annoyingly, some of the anchors in the raw HTML are kebab-case and some are camelCase, but converted to kebab-case dynamically - and both forms work. I've fixed these to not break the linkcheck.

as a sidenote, the changelog shell script generates these links. I simply relied on the github redirects as the numbers are unique identifiers and the script cannot easily distinguish between issues and PRs.

Yup. And this is reasonable. I just remembered that linkcheck_allowed_redirects is now available. I had wanted it to go further so that you could substitute matches from the source pattern into the target pattern to be more strict, but it'll at least allow us to avoid having to change manually in this case. See sphinx-doc/sphinx#6525 for more details. I've added this setting and reverted some of the redirects to the original. Note that this is also useful for documentation, e.g. Django, where something like /en/stable/.* redirects to /en/4.0/.* and we always want to point to the latest stable version.

@tfranzel
Copy link
Owner

I see. Thanks for putting in the work!

Django, where something like /en/stable/.* redirects to /en/4.0/.* and we always want to point to the latest stable version.

yes, noticed that one too.

@tfranzel tfranzel merged commit 2f52d00 into tfranzel:master Apr 21, 2022
@ngnpope ngnpope deleted the patch-1 branch April 21, 2022 09:36
tfranzel added a commit that referenced this pull request Apr 28, 2022
@tfranzel
fix codecov badge url issue on github #713
b3e00be
@tfranzel
Copy link
Owner

for some reason, github did not like the new badge url although request by hand worked without issues. restored old version.

@ngnpope
Copy link
Contributor Author

ngnpope commented Apr 28, 2022

for some reason, github did not like the new badge url although request by hand worked without issues. restored old version.

Huh. Weird. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tfranzel tfranzel tfranzel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ngnpope @tfranzel