Skip to content

Update Sphinx and Sphinx-AutoAPI #20079

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
Merged
ashb merged 3 commits into from
Dec 9, 2021
Merged

Update Sphinx and Sphinx-AutoAPI #20079

ashb merged 3 commits into from
Dec 9, 2021

Conversation

@ashb
Copy link
Member

@ashb ashb commented Dec 6, 2021

I haven't looked in detail how much the output differs, so I am marking this for draft right now.

Spelling check might not pass, but the build was passing locally.

The words added to the spelling dictionary are because Sphinx now picks up typehints (yay!)

@boring-cyborg boring-cyborg bot added area:core-operators provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers area:Scheduler including HA (high availability) scheduler kind:documentation provider:google Google (including GCP) related issues labels Dec 6, 2021
@ashb ashb requested review from potiuk and uranusjr and removed request for uranusjr December 6, 2021 16:51
@ashb ashb mentioned this pull request Dec 6, 2021
Closed
@potiuk
Copy link
Member

potiuk commented Dec 6, 2021

👯

@ashb
Copy link
Member Author

ashb commented Dec 6, 2021

Not sure what changed this time, maybe I never checked on v1.8.

uranusjr
uranusjr reviewed Dec 7, 2021
View reviewed changes
airflow/models/dag.py Outdated
Comment on lines -2940 to +2964
Copy link
Member

@uranusjr uranusjr Dec 7, 2021

Choose a reason for hiding this comment

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

Parametrize seems more widespread? That’s the spelling Pytest uses as well. I don’t know.

Copy link
Member Author

@ashb ashb Dec 7, 2021

Choose a reason for hiding this comment

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

But not https://pypi.org/project/parameterized/ -- and this spelling was in the dictionary, the other wasn't.

(As a British English speaker param-et-er-ized is how I say it, so I was happy to go along with this.)

@uranusjr uranusjr mentioned this pull request Dec 7, 2021
Closed
@ashb
Copy link
Member Author

ashb commented Dec 7, 2021

Right, now we just need to look at the new output and see if we are happy with it 😁

@uranusjr
Copy link
Member

uranusjr commented Dec 7, 2021

The biggest change (excluding changes caused by the underlying dependency upgrades, which isn’t visible in the source) is exampleinclude -> literalinclude. Any specific reason for those?

@ashb
Copy link
Member Author

ashb commented Dec 7, 2021

The biggest change (excluding changes caused by the underlying dependency upgrades, which isn’t visible in the source) is exampleinclude -> literalinclude. Any specific reason for those?

exampleinclude was only ever designed to work with .py files, and sphinx got more specific about the "register_source" call we were doing there -- it errored on later Sphinx. literalinclude does what we want for those files, so it seemed easier to use that directive than change our exampleinclude to cope with non .py files

@ashb ashb mentioned this pull request Dec 7, 2021
Merged
@ashb ashb marked this pull request as ready for review December 7, 2021 13:01
kaxil
kaxil reviewed Dec 7, 2021
View reviewed changes
airflow/providers/google/cloud/utils/credentials_provider.py Outdated
Copy link
Member

@kaxil kaxil Dec 7, 2021

Choose a reason for hiding this comment

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

TIL -- didn't knew we could do this in docstrings too

Copy link
Member Author

@ashb ashb Dec 7, 2021

Choose a reason for hiding this comment

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

Yup, Doc strings are "just" rst :)

@kaxil
Copy link
Member

kaxil commented Dec 7, 2021

Conflicts !

@ashb ashb force-pushed the update-sphinx-autoapi branch from 56fe05b to 9c5ddab Compare December 7, 2021 13:14
@ashb
Copy link
Member Author

ashb commented Dec 7, 2021 •
edited
Loading

Easy one to resolve -- it was TP's :sphinx-autoapi-skip: change that I'd already accounted for in docs/conf.py here

@ashb
Copy link
Member Author

ashb commented Dec 7, 2021

I might have a temporary fix for the failing build, and I'd rather give this PR time for a few more eyes, that or someone to have looked At the built docs and say "yes, this output LGTM"

@ashb
Copy link
Member Author

ashb commented Dec 9, 2021

I've given this a cursor look, and it seems okay content wise -- possibly a few styling things we could fix up such as this

Before:
image

After:
image

ashb added 3 commits December 9, 2021 11:16
@ashb
Update sphinx, sphinx-autoapi and sphinxcontrib-spelling
c3ea228 
We were stuck on an old version of Sphinx AutoAPI for a long while as
more recent versions wouldn't build Airflow's docs, but that seems to
have finally been resolved.

We can remove the run_patched_sphinx.py as that was included in
sphinx-autoapi 1.1
@ashb
Fix doc rendering glitch in Google provider utils
2d5d8dd
@ashb
Remove duplicated link from cncf-kubernetes provider index
efd7eee
@ashb ashb force-pushed the update-sphinx-autoapi branch from 9c5ddab to efd7eee Compare December 9, 2021 11:20
@github-actions
Copy link

github-actions bot commented Dec 9, 2021

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 9, 2021
@potiuk
Copy link
Member

potiuk commented Dec 9, 2021 •
edited
Loading

I've given this a cursor look, and it seems okay content wise -- possibly a few styling things we could fix up such as this

I think we have quite a few other styling issues (most notably the cursed expanding-beyond-screen wide tables with vertical scrolling which make most of the wide tables hidden to a casual user) so I think we should fix those in a separate PR.

Maybe even someone from the UI team, could take a look because of course:

image

@ashb ashb merged commit fa96b09 into apache:main Dec 9, 2021
@ashb ashb deleted the update-sphinx-autoapi branch December 9, 2021 14:02
mik-laj
mik-laj reviewed Dec 9, 2021
View reviewed changes
@ashb ashb mentioned this pull request Dec 9, 2021
Merged
@potiuk potiuk added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jan 22, 2022
@potiuk potiuk added this to the Airflow 2.2.4 milestone Jan 22, 2022
potiuk pushed a commit that referenced this pull request Jan 22, 2022
@ashb @potiuk
Update Sphinx and Sphinx-AutoAPI (#20079)
f59c150 
We were stuck on an old version of Sphinx AutoAPI for a long while as
more recent versions wouldn't build Airflow's docs, but that seems to
have finally been resolved.

We can remove the run_patched_sphinx.py as that was included in
sphinx-autoapi 1.1

* Fix doc rendering glitch in Google provider utils

* Remove duplicated link from cncf-kubernetes provider index

(cherry picked from commit fa96b09)
jedcunningham pushed a commit that referenced this pull request Jan 27, 2022
@ashb @jedcunningham
Update Sphinx and Sphinx-AutoAPI (#20079)
96f88a6 
We were stuck on an old version of Sphinx AutoAPI for a long while as
more recent versions wouldn't build Airflow's docs, but that seems to
have finally been resolved.

We can remove the run_patched_sphinx.py as that was included in
sphinx-autoapi 1.1

* Fix doc rendering glitch in Google provider utils

* Remove duplicated link from cncf-kubernetes provider index

(cherry picked from commit fa96b09)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@uranusjr uranusjr uranusjr left review comments

@mik-laj mik-laj mik-laj left review comments

@potiuk potiuk potiuk approved these changes

@kaxil kaxil kaxil approved these changes

@turbaszek turbaszek Awaiting requested review from turbaszek

@vikramkoka vikramkoka Awaiting requested review from vikramkoka

@XD-DENG XD-DENG Awaiting requested review from XD-DENG

Assignees

No one assigned

Labels

area:providers area:Scheduler including HA (high availability) scheduler changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge kind:documentation provider:cncf-kubernetes Kubernetes (k8s) provider related issues provider:google Google (including GCP) related issues

Projects

None yet

Milestone

Airflow 2.2.4

Development

Successfully merging this pull request may close these issues.

5 participants

@ashb @potiuk @uranusjr @kaxil @mik-laj