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

Google provider docstring improvements #31731

Merged
merged 26 commits into from
Jun 22, 2023

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Jun 6, 2023

Related suggestions found when reviewing #31422.

@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:providers provider:google Google (including GCP) related issues labels Jun 6, 2023
@uranusjr
Copy link
Member Author

uranusjr commented Jun 8, 2023

Apparently I broke something but I can’t figure out what’s going on. The error message is quite unhelpful too 😕

@potiuk
Copy link
Member

potiuk commented Jun 8, 2023

Apparently I broke something but I can’t figure out what’s going on. The error message is quite unhelpful too confused

You can thank sphinx-autoapi for those unhelpful messages, no-one can do much about it.

 WARNING: Definition list ends without a blank line; unexpected unindent.

Apparently the docstring generated rst (in _api folder) contains invalid .rst after your change with missing blank like.

@potiuk
Copy link
Member

potiuk commented Jun 8, 2023

File path: apache-airflow-providers-google/_api/airflow/providers/google/cloud/hooks/bigquery/index.rst

You can do breeze build-docs --package-filter apache-airflow-providers-google and look at the generated .rst to find out the iseue. Ths is as much as can be done there.

@uranusjr
Copy link
Member Author

I did that, the problem is I can’t even figure out where the generated rst has any issues. The files just look good to me. 🤯

@potiuk
Copy link
Member

potiuk commented Jun 16, 2023

I did that, the problem is I can’t even figure out where the generated rst has any issues. The files just look good to me. exploding_head

Been there a few times. What worked for me before is doing bisecting and removing parts of the change and figuring out which one is causing it... Not ideal, but works eventually.

And there are hopes we might get some better error messages - I see some activity happening on readthedocs/sphinx_rtd_theme#1463 where people started to complain big time on sphinx_rtd_theme not being updated to Sphinx 7 and THIS PR readthedocs/sphinx_rtd_theme#1464 has just gotten green, so maybe we should also chime in there ?

I think we really need it.

Let me take a look maybe I can figure that one out quickly.

@potiuk
Copy link
Member

potiuk commented Jun 16, 2023

BTW. breeze build-docs --package-filter apache-airflow-providers-googe --one-pass-only --docs-ony will give you the fastest iteration on this (but the fact that google is about 50% of all provider's code, does not make it exactly fast

@@ -618,10 +610,10 @@ def create_external_table(
:param labels: A dictionary containing labels for the BiqQuery table.
:param description: A string containing the description for the BigQuery table.
:param encryption_configuration: [Optional] Custom encryption configuration (e.g., Cloud KMS keys).
**Example**: ::
.. code-block:: python
Copy link
Member

Choose a reason for hiding this comment

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

Result of bisecting - you miss empty line here. Also indenting is not consistent with other places but it seem to not cause the problem.

@potiuk
Copy link
Member

potiuk commented Jun 16, 2023

I checked the bigquery - and the result are missing empty lines before the code-blocks. I leave the other file for you, bisecting might at least tell you what's wrong.

And more complaints and encourgement here will not hurt - I believe Sphinx 7 wil show the right place for the error. Maybe also changing or vendoring the theme and fixing it for Sphinx 7 would be a good option if someone woudl like to take that on.

@potiuk
Copy link
Member

potiuk commented Jun 16, 2023

Created #31963

@potiuk
Copy link
Member

potiuk commented Jun 16, 2023

Added comment here as well - readthedocs/sphinx_rtd_theme#1463 (comment) @uranusjr -> I'd appreciate some support/words of encouragement for sphinx_rtd_them developers.

I really feel your pain BTW.

@uranusjr
Copy link
Member Author

FWIW we really should switch away from sphinx-autoapi as well (in favor of sphinx.ext.autodoc), I think that’s another blocker for Sphinx 7 and generally a pain to interact with.

@potiuk
Copy link
Member

potiuk commented Jun 16, 2023

sphinx.ext.autodoc)

Updated in #31963

@jedcunningham jedcunningham merged commit fd116cc into apache:main Jun 22, 2023
@jedcunningham jedcunningham deleted the docstring-improve-google branch June 22, 2023 14:20
ferruzzi pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:cncf-kubernetes Kubernetes provider related issues provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants