Skip to content

Discourage intermediate values for context managers#74

Open
schrockn wants to merge 1 commit intomasterfrom
no-intermediate-vars-for-context-managers
Open

Discourage intermediate values for context managers#74
schrockn wants to merge 1 commit intomasterfrom
no-intermediate-vars-for-context-managers

Conversation

@schrockn
Copy link
Copy Markdown
Member

@schrockn schrockn commented Apr 3, 2026

Summary & Motivation

Test Plan

Changelog

The changelog is generated by an agent that examines merged PRs and
summarizes/categorizes user-facing changes. You can optionally replace
this text with a terse description of any user-facing changes in your PR,
which the agent will prioritize. Otherwise, delete this section.

Copy link
Copy Markdown
Member Author

schrockn commented Apr 3, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 3, 2026

Greptile Summary

This PR adds a new section to dignified-python-core.md that discourages extracting context managers into intermediate variables, guiding developers to keep context managers inline within with statements for explicitness.

  • Adds a "Keep Context Managers Inline in with Statements" subsection under the existing code style guidance.
  • Provides a CORRECT/WRONG code example pattern using nullcontext() as a fallback for conditional context managers.
  • Describes why the inline style is preferred: the __enter__/__exit__ lifecycle is visually co-located with the with keyword, and offers the helper-function escape hatch for complex cases.
  • One minor inaccuracy: the second "CORRECT" example comment reads "Multiple conditional context managers," but the example only demonstrates a single conditional context manager.

Confidence Score: 5/5

Safe to merge — documentation-only change with one minor comment wording inaccuracy.

The change is purely documentation (a Markdown style guide). The advice is technically sound, the code examples are valid Python, and the guidance is consistent with the rest of the file. The only issue found is a misleading inline comment in the second example, which has no runtime impact.

No files require special attention beyond the minor comment wording fix flagged inline.

Important Files Changed

Filename Overview
skills/dignified-python/skills/dignified-python/dignified-python-core.md Adds a new section discouraging extraction of context managers to intermediate variables; content is technically sound with one minor comment inaccuracy (second example labelled "Multiple conditional context managers" but only shows one).

Sequence Diagram

sequenceDiagram
    participant Code as User Code
    participant CM as Context Manager
    note over Code,CM: PREFERRED: Inline in with statement
    Code->>CM: with (cm_a if cond else nullcontext())
    CM->>CM: __enter__()
    Code->>Code: do_work()
    CM->>CM: __exit__()

    note over Code,CM: DISCOURAGED: Extracted to intermediate variable
    Code->>CM: cm = cm_a if cond else nullcontext()
    Code->>CM: with cm:
    CM->>CM: __enter__()
    Code->>Code: do_work()
    CM->>CM: __exit__()
Loading

Reviews (1): Last reviewed commit: "cp" | Re-trigger Greptile

Comment on lines +322 to +323
# CORRECT: Multiple conditional context managers
with (lock if thread_safe else nullcontext()):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Misleading example comment

The comment says "Multiple conditional context managers," but the example only shows a single conditional context manager (with a nullcontext() fallback). This could confuse readers into thinking the guidance applies specifically to multiple context managers rather than any conditional context manager.

Suggested change
# CORRECT: Multiple conditional context managers
with (lock if thread_safe else nullcontext()):
# CORRECT: Conditional context manager with nullcontext fallback
with (lock if thread_safe else nullcontext()):

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant