Move YAPF style setting to a project-wide .style.yapf file#1038
Move YAPF style setting to a project-wide .style.yapf file#1038mhucka wants to merge 2 commits intotensorflow:masterfrom
.style.yapf file#1038Conversation
Set the YAPF style to Google style.
There was a problem hiding this comment.
Code Review
This pull request adds a .style.yapf configuration file to set the Python formatting style to Google. The review feedback suggests updating existing formatting scripts to remove hardcoded style flags, ensuring this new file serves as the single source of truth, and recommends adding a .clang-format file for C++ consistency.
.style.yapf.style.yapf file
Moved the style setting to the config file.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request centralizes the YAPF configuration by introducing a .style.yapf file and removing redundant style flags from the formatting scripts. Feedback suggests invoking yapf via python3 -m in format_check.sh for better environment consistency and providing the filename parameter to the YAPF API in format_ipynb.py to ensure the new configuration file is properly detected.
| if [[ "${changed_line_ranges}" != "--lines=0-0 " ]]; then | ||
| # Do the formatting. | ||
| results=$(yapf --style=google --diff "${changed_file}" ${changed_line_ranges}) | ||
| results=$(yapf --diff "${changed_file}" ${changed_line_ranges}) |
There was a problem hiding this comment.
For consistency with scripts/format_all.sh and to ensure the correct environment is used, it is recommended to invoke yapf using python3 -m yapf. This avoids potential issues where the yapf executable in the PATH might be associated with a different Python interpreter or version than the one intended for the project.
| results=$(yapf --diff "${changed_file}" ${changed_line_ranges}) | |
| results=$(python3 -m yapf --diff "${changed_file}" ${changed_line_ranges}) |
| try: | ||
| fmt_lines = yapf.yapf_api.FormatCode(''.join(lines), | ||
| style_config="google")[0] | ||
| fmt_lines = yapf.yapf_api.FormatCode(''.join(lines))[0] |
There was a problem hiding this comment.
When using the yapf API without an explicit style_config, it is best practice to provide the filename argument. This allows yapf to search for the nearest configuration file (like the newly added .style.yapf) relative to the file being formatted, making the script more robust if run from different directories or if different parts of the project eventually require different styles.
| fmt_lines = yapf.yapf_api.FormatCode(''.join(lines))[0] | |
| fmt_lines = yapf.yapf_api.FormatCode(''.join(lines), filename=fname)[0] |
Scripts and workflows that run
yapfcurrently each add the option--style=googleto the invocation ofyapf. To follow best practices, this PR adds a.style.yapfconfiguration file at the top level of the project to set the style, and removes the flag from where it was added on the command line explicitly.The use of a config file has the following benefits:
yapfdirectly on the command line for quick checks, or write new scripts that useyapf.Removing the flag from the scripts is not strictly necessary (everything will work the same if there is both a config file and the flag is passed to
yapf). However, if a config file is used, it's better for maintainability and consistency to also remove the flag from individual invocations.