27 Refactor project structure for Poetry and improve flowchart capability #28
Conversation
📝 WalkthroughWalkthroughAdds Poetry packaging, CI, and pre-commit hooks; removes Flake8 config; introduces a new AST-based flowchart generator (serpent/core.py), a Streamlit app entry (serpent/app.py), tests, README updates, and moves/refactors the legacy generator. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant StreamlitApp as serpent.app
participant Core as serpent.core
participant Graphviz as dot
User->>Browser: Open app, paste code, click Generate
Browser->>StreamlitApp: UI event with code string
StreamlitApp->>Core: call generate_graphviz_flowchart(code_str, title)
Core->>Core: parse AST & traverse (PythonFlowchartGV)
Core->>Graphviz: produce DOT source (Digraph)
alt dot installed
StreamlitApp->>Graphviz: render PNG from DOT
Graphviz-->>StreamlitApp: PNG bytes
StreamlitApp-->>Browser: display PNG and offer PNG download
else dot missing
StreamlitApp-->>Browser: display DOT source and offer DOT download with warning
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
serpent_scripts/serpent_graphviz.py (1)
362-390: Remove duplicate code fromserpent_scripts/serpent_graphviz.py.The
generate_graphviz_flowchartfunction andPythonFlowchartGVclass are duplicated in this file andserpent/core.py. Sinceserpent/__init__.pyimports fromserpent.coreand theserpent_scripts/serpent_graphviz.pymodule is not imported anywhere in the codebase, this file contains unused duplicate code that should be removed.README.md (1)
10-10: Fix the README image reference path on line 10.
examples/serpent_banner_white_bg.pngdoes not exist; the file is located inassets/serpent_banner_white_bg.png. Update the reference on line 10 to use the correct path. (Lines 53 and 69 reference valid image files in the examples directory and do not need changes.)
🤖 Fix all issues with AI agents
In @.pre-commit-config.yaml:
- Around line 10-29: The isort hook in .pre-commit-config.yaml is using an
invalid rev "6.1.0"; update the isort repo entry (the hook with id: isort and
name: isort (python)) to reference a valid version on PyPI such as "6.0.1" or
"7.0.0" by replacing the rev value accordingly so the pre-commit hook resolves
correctly.
In @packages.txt:
- Line 1: Delete the legacy packages.txt file (which currently lists "graphviz")
from the repository; ensure no build scripts, CI config, or deployment manifests
reference packages.txt (search for "packages.txt" and remove any references),
and rely on the existing pyproject.toml/poetry.lock entries (e.g., graphviz) for
dependency management instead.
In @serpent/app.py:
- Around line 206-213: Replace the blind top-level exception handler that shows
raw exception details to users: in the except Exception block (and keep the
SyntaxError user message unchanged), log the full traceback server-side using
logging.exception(...) or traceback.format_exc() before returning a user-safe
message via st.error without the raw exception text; specifically update the
generic except Exception handler that currently calls st.error(f"...{e}") to
call logging.exception("Unexpected error in <function/module>") and then
st.error("An unexpected error occurred. Please try again or contact support.")
so diagnostics are recorded but internal details aren't exposed to users.
- Around line 179-193: The TODO comment indicating PNG download is unimplemented
is stale; remove or replace it with an accurate comment describing that when
shutil.which("dot") is truthy the code generates PNG bytes via
graph.pipe(format="png") and provides a download via st.download_button
(file_name uses chart_title); update the comment to reflect the implemented PNG
download behavior or delete the TODO so future readers aren't confused.
- Around line 137-147: Sanitize chart_title before using it to build file_name:
normalize to lowercase, remove or replace unsafe characters (e.g., strip path
separators, control chars), collapse whitespace to single hyphens/underscores,
truncate to a safe max length, and if the result is empty use a default like
"flowchart" (optionally append a timestamp/uuid for uniqueness); then construct
file_name using that sanitized slug + proper extension (e.g., .png/.dot). Apply
this change to all places that create file_name from chart_title (the file_name
assignments that currently use chart_title.replace(...).lower()) so both PNG and
DOT exports are safe.
In @serpent/core.py:
- Around line 273-301: The visitor methods visit_Break and visit_Continue
declare a parameter named node that is unused, triggering Ruff ARG002; rename
the unused parameter to _node in both method signatures (visit_Break(self,
_node: ast.Break) and visit_Continue(self, _node: ast.Continue)) so the linter
knows it’s intentionally unused, or alternatively add an explicit del node or a
trailing comment pragma, but prefer renaming to _node to be concise and
consistent with other visitor methods.
- Around line 136-201: The True/False labels are lost when a branch creates no
nodes because visit_If relies on the global next_edge_label consumed by the
first created node; change the flow to propagate edge labels with source tuples
instead: modify visit_If to set branch starters as (decision_node_id,"True") and
(decision_node_id,"False") when a branch may produce no nodes, collect
last_nodes as (node_id,label) pairs instead of raw ids, and update
node-creation/connection logic (the code path in new_node creation and consumers
of self.last_nodes such as visit_Pass/visit_Expr and places that currently set
self.last_nodes = [decision_node_id]) to accept and consume a tuple label when
present (clearing the label only when it's applied), so edge labels survive
empty branches and are attached to the merge edges.
- Around line 218-272: The loop handler can leave next_edge_label="True" unused
for empty bodies and passes None as an edge label; change the True-path seed to
use a tuple so the "True" label is attached even when the body yields no nodes
(set self.last_nodes = [(condition_node, "True")] instead of [condition_node]),
and when creating back-edges with self.graph.edge(src, condition_node,
label=lbl) only include the label argument if lbl is not None/empty (e.g., call
self.graph.edge(src, condition_node) when lbl is None or use "" instead), also
keep the existing loop_stack usage (loop_stack entries, loop_ctx,
condition_node) and ensure next_edge_label is reset as before.
🧹 Nitpick comments (9)
.pylintrc (1)
1-5: Restructure .pylintrc to use proper INI format or consolidate configuration into pyproject.toml.The
.pylintrcfile uses non-standard syntax. The linepylint: max-line-length=120is a directive comment format (typically used inline in.pyfiles), not valid in a configuration file. A proper.pylintrcrequires INI format with section headers such as[DESIGN]or[MESSAGES CONTROL]. Alternatively, since the project uses Poetry, consider consolidating pylint settings intopyproject.tomlunder[tool.pylint]to maintain a single source of tool configuration..github/workflows/test.yml (2)
14-19: Update GitHub Actions to current versions.The
actions/checkout@v3andactions/setup-python@v4are outdated. Using older versions may cause compatibility issues with newer GitHub Actions runners.♻️ Proposed fix
- - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: Set up Python - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: python-version: "3.10"
21-29: Consider adding dependency caching for faster CI runs.The workflow reinstalls all dependencies on every run. Adding Poetry caching can significantly speed up CI execution.
♻️ Example with caching
- name: Set up Python uses: actions/setup-python@v5 with: python-version: "3.10" cache: 'pip' - name: Install Poetry run: pip install poetry - name: Cache Poetry dependencies uses: actions/cache@v4 with: path: ~/.cache/pypoetry key: ${{ runner.os }}-poetry-${{ hashFiles('**/poetry.lock') }} restore-keys: | ${{ runner.os }}-poetry- - name: Install dependencies run: poetry install - name: Run tests run: poetry run pytest tests/serpent_scripts/serpent_graphviz.py (1)
69-75: Consider using a parameter instead of instance state for edge labels.The
next_edge_labelpattern usinggetattr/setattris a bit fragile. If a visit method forgets to clear it, labels could leak to unintended edges. Consider passing the edge label explicitly through visit calls or documenting this invariant clearly.tests/test_flowchart.py (1)
44-68: Consider expanding test coverage.The current tests provide good baseline coverage. Consider adding tests for:
continuestatement handlingelifchains (nested else-if)forloops- Nested loops with break/continue
- Multiple functions in one code block
Example additional test:
def test_continue_statement(): """Test continue statement in loop.""" code = """ while True: if x: continue print("After continue check") """ graph = generate_graphviz_flowchart(code) source = graph.source assert "continue" in source assert "While: True" in sourceREADME.md (1)
71-137: Align “Development / Linter” commands with Poetry-based workflow.Since “Getting Started” is Poetry-first (Line 84-116), “Running the Linter” (Line 134-137) should likely be
poetry run flake8 .(or whatever tool is actually configured via pre-commit) to avoid requiring a global install.serpent/core.py (2)
118-135: Skip visiting unreachable statements after flow terminators (return/break/continue).After
visit_Return/visit_Break/visit_Continue,self.last_nodesis set to[](Line 284-285, 298-299, 310-311). Continuing tovisit()subsequent statements in the same block will create disconnected nodes (becausenew_node()will have nosources). If you want unreachable code omitted (typical flowchart expectation), break out of the statement loop whenlast_nodesbecomes empty.Also applies to: 158-176, 242-248
364-392: Consider returning SyntaxError details from the public API (optional).
generate_graphviz_flowchartswallows the exception and returns a generic “Syntax Error” node (Line 375-387). The Streamlit app currently re-parses just to show details. Consider capturing the exception message in the error node (or offering an option to surface details) so callers don’t need to parse twice.serpent/app.py (1)
22-31: Asset fallback is good, but consider packaging-safe asset loading for installs.
assets_dir = Path(__file__).parent.parent / "assets"(Line 22) is fine for a repo checkout, but can break depending on packaging/layout. If this is intended to work when installed as a package, preferimportlib.resourcesfor asset retrieval (and keep the current fallback).Also applies to: 33-38
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
assets/serpent_banner_transparent.pngis excluded by!**/*.pngassets/serpent_banner_white_bg.pngis excluded by!**/*.pngassets/serpent_logo_transparent.pngis excluded by!**/*.pngpoetry.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.flake8.github/workflows/test.yml.gitignore.pre-commit-config.yaml.pylintrc.streamlit/config.tomlREADME.mdpackages.txtpyproject.tomlserpent/__init__.pyserpent/app.pyserpent/core.pyserpent_scripts/serpent_graphviz.pytests/test_flowchart.py
💤 Files with no reviewable changes (1)
- .flake8
🧰 Additional context used
🧬 Code graph analysis (2)
serpent/__init__.py (2)
serpent/core.py (1)
generate_graphviz_flowchart(364-392)serpent_scripts/serpent_graphviz.py (1)
generate_graphviz_flowchart(362-390)
serpent/app.py (2)
serpent/core.py (1)
generate_graphviz_flowchart(364-392)serpent_scripts/serpent_graphviz.py (1)
generate_graphviz_flowchart(362-390)
🪛 actionlint (1.7.10)
.github/workflows/test.yml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
17-17: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 Ruff (0.14.10)
serpent/core.py
273-273: Unused method argument: node
(ARG002)
288-288: Unused method argument: node
(ARG002)
serpent_scripts/serpent_graphviz.py
271-271: Unused method argument: node
(ARG002)
286-286: Unused method argument: node
(ARG002)
serpent/app.py
210-210: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (8)
.gitignore (1)
1-5: LGTM.Standard .gitignore configuration appropriate for a Python project with Poetry/build artifacts.
.streamlit/config.toml (1)
1-7: LGTM!The Streamlit theme configuration looks correct with valid settings for base theme, colors, and font.
pyproject.toml (1)
1-34: LGTM!The Poetry configuration is well-structured with appropriate dependency specifications, dev tooling, and CLI entry point. The pylint message controls are reasonable for this project type.
serpent_scripts/serpent_graphviz.py (3)
163-165: Emptyifbody handling may produce unexpected edges.When
node.bodyis empty (which is syntactically invalid in Python but could occur in partial AST manipulation),true_end_nodesis set to[decision_node_id]. This means the "True" edge won't be labeled sincenext_edge_labelwas set but never consumed by anew_nodecall. The behavior is acceptable since empty bodies are rare, but worth noting.
271-298: Unusednodeparameter is intentional for AST visitor pattern.The static analysis flags
nodeas unused invisit_Breakandvisit_Continue. This is expected since the AST visitor pattern requires this signature, andbreak/continuestatements carry no additional data. The current implementation is correct.
300-308: LGTM!The
visit_Returncorrectly terminates flow by clearinglast_nodes, preventing subsequent statements from connecting to it.serpent/__init__.py (1)
1-5: LGTM!Clean package initialization that exposes the primary public API. Consider also exporting
PythonFlowchartGVin__all__if users need direct access to the visitor class for customization.tests/test_flowchart.py (1)
8-21: LGTM!Good basic test for linear function flow. Consider adding a test for functions with parameters to verify they're displayed correctly.
Asifdotexe
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
serpent/app.py (2)
174-179: Consider removing redundant AST parsing.The code calls
ast.parse(code)at line 176 for validation, butgenerate_graphviz_flowchartalso parses the code internally. For valid code, this results in parsing twice.You could either:
- Remove the validation here and let
generate_graphviz_flowcharthandle syntax errors (it returns an error graph), or- Modify
generate_graphviz_flowchartto accept a pre-parsed AST treeThe current approach is functional but slightly inefficient.
181-185: Consider extracting filename sanitization to a helper function.The sanitization logic works correctly but could be more maintainable as a separate helper function, especially if you need to reuse this logic elsewhere.
♻️ Optional refactor
def sanitize_filename(title: str, max_length: int = 50, fallback: str = "flowchart") -> str: """Sanitize a title for use as a filename.""" safe = re.sub(r"[^a-z0-9_\-]", "", title.lower().replace(" ", "_")) safe = re.sub(r"_+", "_", safe).strip("_") return safe[:max_length] or fallbackThen use it:
safe_title = sanitize_filename(chart_title)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.pre-commit-config.yamlserpent/app.pyserpent/core.py
🚧 Files skipped from review as they are similar to previous changes (2)
- serpent/core.py
- .pre-commit-config.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
serpent/app.py (2)
serpent/core.py (1)
generate_graphviz_flowchart(364-392)serpent_scripts/serpent_graphviz.py (1)
generate_graphviz_flowchart(362-390)
🔇 Additional comments (9)
serpent/app.py (9)
1-22: LGTM!The imports are well-organized and the module docstring clearly describes the app's purpose.
24-32: Good fallback handling for missing assets.The graceful fallback to
Nonewhen assets are missing is a solid choice for package distribution scenarios.
34-53: LGTM!The page configuration and custom styling are appropriate. The empathic comments add a nice touch.
55-66: LGTM!The placeholder code is engaging and demonstrates the tool's capabilities effectively.
76-112: LGTM!The UI setup is user-friendly with good fallback handling and helpful guidance for users.
117-149: LGTM!The input column is well-structured with clear labels and helpful tooltips.
187-211: LGTM!The conditional download options based on Graphviz availability provide excellent UX with clear messaging and helpful fallbacks.
213-221: LGTM!The exception handling is well-structured with appropriate logging and user-friendly error messages.
223-233: LGTM!The entry point follows standard Python conventions and the footer encourages community engagement.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_app_smoke.py (1)
11-15: Add bounds checking for caption assertion.The test directly indexes
at.caption[0]without verifying that a caption element exists. If the app doesn't render a caption (e.g., based on the "conditional banner presence" mentioned in line 13), this will raise anIndexErrorrather than a more informativeAssertionError.Additionally, the hardcoded string comparison is brittle and will break with any minor text changes to the app.
♻️ Suggested improvements for robustness
def test_app_startup(): """ Smoke test to verify the app starts up without errors. """ at = AppTest.from_file(app.__file__).run() - # Check if the caption is correct (title is conditional on banner presence) - assert at.caption[0].value == "Turn your Python functions into clear, standard flowcharts in a few clicks." + # Check if the caption is correct (title is conditional on banner presence) + assert len(at.caption) > 0, "Expected at least one caption element" + assert at.caption[0].value == "Turn your Python functions into clear, standard flowcharts in a few clicks." assert not at.exception
The goal of this pull request was to ensure that we added support for break, continue and labelled the true and false branching of the conditional statement
Additionally we also pivoted to using poetry and restructured the directory structure for it
Performed code linting and cleanup, improved docstring coverage and added empathic block comments for my future miserable self as I will end up forgetting why I wrote that logic in that way in the first place
Also added TODO comment to mark additional items (probably worth looking into a bot to handle the todo?)
Adding a testing workflow to github actions
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores / CI
✏️ Tip: You can customize this high-level summary in your review settings.