Skip to content

fix: rename shadowed 'format' builtin in _render_graphviz#759

Merged
skrawcz merged 1 commit into
apache:mainfrom
dashitongzhi:fix/rename-shadowed-format-builtin
May 11, 2026
Merged

fix: rename shadowed 'format' builtin in _render_graphviz#759
skrawcz merged 1 commit into
apache:mainfrom
dashitongzhi:fix/rename-shadowed-format-builtin

Conversation

@dashitongzhi
Copy link
Copy Markdown
Contributor

Problem

In burr/core/graph.py, the function _render_graphviz() uses format as a local variable name (lines 98, 100). This shadows Python's built-in format() function, which can lead to confusion and potential bugs if the builtin is needed within the function's scope. Many linters (flake8 A001/A002, pylint W0622) flag this as a code smell.

Fix

Renamed the local variable from format to fmt across all 5 occurrences within the function:

# Before
format = output_file_path.suffix.partition(".")[-1]
format = "png"
graphviz_obj.render(path_without_suffix, format=format, view=view)
pathlib.Path(f"{path_without_suffix}.{format}").write_bytes(...)
graphviz_obj.pipe(format=format)

# After
fmt = output_file_path.suffix.partition(".")[-1]
fmt = "png"
graphviz_obj.render(path_without_suffix, format=fmt, view=view)
pathlib.Path(f"{path_without_suffix}.{fmt}").write_bytes(...)
graphviz_obj.pipe(format=fmt)

The keyword argument format= in calls to graphviz_obj.render() and graphviz_obj.pipe() is preserved as-is — only the local variable assignment and its usages are renamed. All behavior remains identical.

In burr/core/graph.py, the function _render_graphviz() used 'format' as
a local variable name, which shadows Python's built-in format() function.
Renamed it to 'fmt' to avoid the shadowing while preserving all existing
behavior.

The variable is used only within the function scope and the rename is
consistent and self-documenting.
Copilot AI review requested due to automatic review settings May 7, 2026 14:06
@github-actions github-actions Bot added the area/core Application, State, Graph, Actions label May 7, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR removes a local variable name that shadows Python’s built-in format() inside burr/core/graph.py:_render_graphviz(), reducing linter warnings and improving readability without changing behavior.

Changes:

  • Renames the local variable format to fmt within _render_graphviz().
  • Updates all uses of the renamed variable while preserving the format= keyword argument in Graphviz calls.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@dashitongzhi
Copy link
Copy Markdown
Contributor Author

Friendly ping for review 🙏 This is a small, focused fix. CI is passing. Thank you!

@dashitongzhi
Copy link
Copy Markdown
Contributor Author

Hi! 👋 This PR has passed all CI checks and looks ready for review. Could a maintainer please take a look when they have a chance? Thank you!

Copy link
Copy Markdown
Contributor

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

LGTM — clean rename of a shadowed builtin. Variable formatfmt in _render_graphviz(), no behavioral change. All graph tests pass.

@skrawcz skrawcz enabled auto-merge (rebase) May 11, 2026 04:59
@skrawcz skrawcz merged commit f7a1750 into apache:main May 11, 2026
28 of 30 checks passed
@skrawcz
Copy link
Copy Markdown
Contributor

skrawcz commented May 11, 2026

The pre-commit black check is failing because with the shorter variable name fmt, the line now fits on a single line. Quick fix:

-        pathlib.Path(f"{path_without_suffix}.{fmt}").write_bytes(
-            graphviz_obj.pipe(format=fmt)
-        )
+        pathlib.Path(f"{path_without_suffix}.{fmt}").write_bytes(graphviz_obj.pipe(format=fmt))

Or just run pre-commit run --all-files and commit the result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/core Application, State, Graph, Actions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants