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

docs(bigquery): fix the broken docs #139

Merged
merged 2 commits into from Jun 19, 2020

Conversation

@HemangChothani
Copy link
Contributor

@HemangChothani HemangChothani commented Jun 18, 2020

Fixes #138

-> I know conf.py file generated by synthtool ,this is temporary patch for bigquery.

-> new release of sphinx 3.1.1 Consider duplicate docstring of Attributes: docstring and property docstring and raised an error of duplicate description so change the docstring from Attributes: to Args for more specific info refer this issue : sphinx-doc/sphinx#7817

@HemangChothani
Copy link
Contributor Author

@HemangChothani HemangChothani commented Jun 18, 2020

Opened a PR in synthtool to update conf.py file template googleapis/synthtool#629

Loading

Copy link
Contributor

@plamut plamut left a comment

I know conf.py file generated by synthtool ,this is temporary patch for bigquery.

How about making the change in synth.py and change the key line in docs/conf.py file automatically?

The fix itself work, though, thanks for that!

Loading

@HemangChothani
Copy link
Contributor Author

@HemangChothani HemangChothani commented Jun 19, 2020

@plamut The PR of synthtool googleapis/synthtool#629 has been merged so i will remove the doc/conf.py change from here, but i don't have an idea about change in synth.py file.

Loading

@plamut
Copy link
Contributor

@plamut plamut commented Jun 19, 2020

@HemangChothani I presume we still need to add the "inherited-members": True option to the docs config, because the synth template contains just the "members": True option? Re-generating the files with synthtool erases that latter part, thus we need to add it manually (through a replacement configured in synth.py)

Mentioning this, because running the synthtool produces the following change (among others):

diff --git docs/conf.py docs/conf.py
index 30dcac5..91b39ac 100644
--- docs/conf.py
+++ docs/conf.py
@@ -43,7 +43,7 @@ extensions = [
 
 # autodoc/autosummary flags
 autoclass_content = "both"
-autodoc_default_options = {"members": True, "inherited-members": True}
+autodoc_default_options = {"members": True}
 autosummary_generate = True

Or do you think we could actually make that change in the docs/conf.py template itself? (don't know if that's suitable for all Python client libs, though)

autodoc_default_options = {"members": True, "inherited-members": True}

Loading

@HemangChothani
Copy link
Contributor Author

@HemangChothani HemangChothani commented Jun 19, 2020

@plamut
In synth.py file can we do that? I am not sure right now as i am struggling with synth config to local machine.

s.replace(
"docs/conf.py",
{"members": True},
{"members": True, "inherited-members": True}
)

I am not sure that it will work for all clients or not , i will test it.

Loading

@plamut
Copy link
Contributor

@plamut plamut commented Jun 19, 2020

@HemangChothani Yes, each library can add custom modifications to the generated files in its synth.py.

The snippet above is the right direction, just mind that the second argument is treated as a regular expression, thus regex special characters need to be escaped.

If you have trouble setting up synth locally, I can post a patch here myself to save time, as I already happen to have everything working locally.

Loading

@q-logic q-logic force-pushed the bigquery_issue_138 branch from e999c20 to 6d95afb Jun 19, 2020
@HemangChothani
Copy link
Contributor Author

@HemangChothani HemangChothani commented Jun 19, 2020

@plamut Thank you for the guidance and support, i am able to run locally an test it and push the code, could you please verify it?

Loading

Copy link
Contributor

@plamut plamut left a comment

Regex pattern needs to be updated... if running the synthtool locally, doesn't it emit a warning that a replacement might no longer be needed?

Loading

synth.py Outdated Show resolved Hide resolved
Loading
noxfile.py Outdated Show resolved Hide resolved
Loading
@q-logic q-logic force-pushed the bigquery_issue_138 branch from 6d95afb to 19adf86 Jun 19, 2020
plamut
plamut approved these changes Jun 19, 2020
Copy link
Contributor

@plamut plamut left a comment

Looks good now. The classes in the generated docs again have fully documented members, and re-generating the code preserves the relevant changes in docs/conf.py.

Thanks for adding the suggested improvements quickly!

Loading

@HemangChothani HemangChothani merged commit 3235255 into googleapis:master Jun 19, 2020
3 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants