feat(docs): publish Client libraries help to python reference documen…#16575
feat(docs): publish Client libraries help to python reference documen…#16575bshaffer wants to merge 6 commits intogoogleapis:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new help package for documentation, including a DocFX helper script and Nox configuration. Key feedback includes concerns about shadowing the built-in help() function, unused imports and parameters in docfx_helper.py, and the need for explicit encoding during file operations. Improvements were also suggested for argument parsing and the removal of legacy Python 2 imports.
| python_requires=">=3.10", | ||
| include_package_data=True, | ||
| zip_safe=False, | ||
| packages=["help"], |
There was a problem hiding this comment.
Naming the package help shadows the Python built-in help() function. This can lead to confusion and subtle bugs when users try to use the built-in help system after importing this package. It is strongly recommended to use a more specific name, such as google_cloud_help.
packages/help/docfx_helper.py
Outdated
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import os |
There was a problem hiding this comment.
The os module is imported but not used in this file. It can be safely removed to keep the code clean.
packages/help/docfx_helper.py
Outdated
| def build_docfx(current_dir, repo_root, docs_map): | ||
| current_dir = pathlib.Path(current_dir) | ||
| repo_root = pathlib.Path(repo_root) |
There was a problem hiding this comment.
The repo_root parameter is accepted and converted to a Path object but is never used within the build_docfx function. If it's not intended for future use, it should be removed from the function signature and body.
| 'gfm', | ||
| format='rst' | ||
| ) | ||
| (output_dir / target_filename).write_text(output) |
There was a problem hiding this comment.
When writing text to a file using write_text, it is recommended to specify the encoding explicitly (e.g., encoding="utf-8") to ensure consistent behavior across different platforms.
(output_dir / target_filename).write_text(output, encoding="utf-8")
packages/help/docfx_helper.py
Outdated
| with open(toc_path, "w") as f: | ||
| yaml.dump(toc, f, default_flow_style=False) |
There was a problem hiding this comment.
To ensure the generated toc.yaml is stable and predictable, programmatically sort the dictionary before dumping it. Also, specify the encoding when opening the file for writing to avoid platform-dependent defaults.
| with open(toc_path, "w") as f: | |
| yaml.dump(toc, f, default_flow_style=False) | |
| with open(toc_path, "w", encoding="utf-8") as f: | |
| yaml.dump(dict(sorted(toc.items())), f, default_flow_style=False) |
References
- To ensure dictionary keys remain sorted without manual effort, programmatically sort the dictionary before returning it (e.g., using dict(sorted(metadata.items()))) instead of relying on manual ordering in the code.
packages/help/docfx_helper.py
Outdated
| curr = sys.argv[1] | ||
| root = sys.argv[2] | ||
| d_map = {} | ||
| for i in range(3, len(sys.argv), 2): | ||
| d_map[sys.argv[i]] = sys.argv[i+1] |
There was a problem hiding this comment.
The manual argument parsing is brittle. If an odd number of arguments is provided for the title/source pairs, the script will crash with an IndexError. Consider using argparse for more robust argument handling and validation.
packages/help/noxfile.py
Outdated
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| from __future__ import absolute_import |
There was a problem hiding this comment.
The from future import absolute_import statement is a legacy from Python 2 compatibility and is no longer necessary in Python 3.10+.
1 participant
b/450298584
Publishes documentation to https://cloud.google.com/python/docs/reference under "Client libraries help".
This currently only contains the
README.rstcontents from the root of this repo. We will expand this to add more once we've confirmed this is deploying as expected.