Conversation
📝 WalkthroughWalkthroughThe PR adds ChangesSQL loader autocommit
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PytestOptions
participant get_config
participant postgresql_proc
participant DatabaseJanitor
participant SQLLoader
PytestOptions->>get_config: Read postgresql_load_autocommit
get_config->>postgresql_proc: Provide load_autocommit
postgresql_proc->>DatabaseJanitor: Construct with autocommit
DatabaseJanitor->>SQLLoader: Load SQL with autocommit
Suggested labels: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pytest_postgresql/janitor.py (1)
158-165: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAvoid breaking existing custom loaders.
Both janitor implementations unconditionally add
autocommitto loader callables, breaking existing loaders that implement the documented five-argument contract.
pytest_postgresql/janitor.py#L158-L165: preserve compatibility for synchronous custom loaders.pytest_postgresql/janitor.py#L263-L271: preserve compatibility for asynchronous and threaded custom loaders.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pytest_postgresql/janitor.py` around lines 158 - 165, Update the synchronous loader invocation in pytest_postgresql/janitor.py:158-165 and the asynchronous/threaded loader invocation in pytest_postgresql/janitor.py:263-271 so custom loaders using the documented five-argument contract are not passed autocommit; preserve autocommit handling for loaders that support it while maintaining compatibility at both call sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pytest_postgresql/factories/noprocess.py`:
- Line 120: Update load_autocommit resolution in
pytest_postgresql/factories/noprocess.py lines 120-120 and
pytest_postgresql/factories/process.py lines 227-227 to distinguish an
explicitly provided factory value, including False, from an omitted value; use
the factory setting when explicit and fall back to config.load_autocommit only
when omitted, preserving identical tri-state precedence in both sites.
- Around line 48-49: Update the factory signature containing load_autocommit and
depends_on to preserve existing positional binding: place load_autocommit after
depends_on, or make the trailing parameters keyword-only. Keep depends_on bound
to its original positional position.
In `@README.rst`:
- Around line 386-391: Update the “Noop process fixture” value for the
load_autocommit/--postgresql-load-autocommit configuration entry in the README
table from “-” to “yes”.
---
Outside diff comments:
In `@pytest_postgresql/janitor.py`:
- Around line 158-165: Update the synchronous loader invocation in
pytest_postgresql/janitor.py:158-165 and the asynchronous/threaded loader
invocation in pytest_postgresql/janitor.py:263-271 so custom loaders using the
documented five-argument contract are not passed autocommit; preserve autocommit
handling for loaders that support it while maintaining compatibility at both
call sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ecf93ba-0f92-4334-b44e-1eef6ad20d97
📒 Files selected for processing (10)
README.rstnewsfragments/902.feature.rstpytest_postgresql/config.pypytest_postgresql/factories/noprocess.pypytest_postgresql/factories/process.pypytest_postgresql/janitor.pypytest_postgresql/plugin.pytests/examples/test_assert_load_autocommit_is_true.pytests/test_janitor.pytests/test_postgres_options_plugin.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.rst (1)
386-391: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the no-process fixture’s configuration precedence.
postgresql_noprocdefaultsload_autocommittoFalseand then treats any non-Nonevalue as an explicit factory override. Consequently,--postgresql-load-autocommitandpytest.iniare ignored unless callers also pass the factory argument, despite this table documenting support.Use a
Nonedefault inpytest_postgresql/factories/noprocess.py, or document the limitation.Proposed fix
- load_autocommit: bool = False, + load_autocommit: bool | None = None,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.rst` around lines 386 - 391, Update the postgresql_noproc factory’s load_autocommit default to None so CLI and pytest.ini configuration can take precedence unless callers explicitly provide a factory override; keep the documented configuration keys and behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pytest_postgresql/factories/process.py`:
- Around line 96-97: Update the postgresql_proc factory signature so
load_autocommit is keyword-only by placing * before it, while preserving the
existing positional bindings for all preceding parameters and keeping
load_autocommit’s behavior unchanged.
---
Outside diff comments:
In `@README.rst`:
- Around line 386-391: Update the postgresql_noproc factory’s load_autocommit
default to None so CLI and pytest.ini configuration can take precedence unless
callers explicitly provide a factory override; keep the documented configuration
keys and behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: af9ffd88-e5a3-411f-adca-e46228bb9e46
📒 Files selected for processing (4)
README.rstnewsfragments/902.break.rstpytest_postgresql/factories/noprocess.pypytest_postgresql/factories/process.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_chaining.py`:
- Line 10: Update the signatures of load_schema, load_data, and load_more_data
so autocommit is keyword-only by placing it after the keyword-only separator;
preserve existing parameter types and behavior, since callers already pass this
argument by keyword.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7f898ab1-4977-4aea-bad0-2eae1b7c6fa7
📒 Files selected for processing (3)
newsfragments/902.break.1.rstpytest_postgresql/factories/noprocess.pytests/test_chaining.py
|
|
||
|
|
||
| def load_schema(host: str, port: int, user: str, dbname: str, password: str | None) -> None: | ||
| def load_schema(host: str, port: int, user: str, dbname: str, password: str | None, autocommit: bool) -> None: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Make autocommit keyword-only.
Ruff flags each new boolean positional parameter (FBT001). The janitor already passes loader arguments by keyword, so this is a safe API clarification:
-def load_schema(host: str, port: int, user: str, dbname: str, password: str | None, autocommit: bool) -> None:
+def load_schema(host: str, port: int, user: str, dbname: str, password: str | None, *, autocommit: bool) -> None:Apply the same change to load_data and load_more_data.
Also applies to: 19-19, 29-29
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 10-10: Boolean-typed positional argument in function definition
(FBT001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_chaining.py` at line 10, Update the signatures of load_schema,
load_data, and load_more_data so autocommit is keyword-only by placing it after
the keyword-only separator; preserve existing parameter types and behavior,
since callers already pass this argument by keyword.
Source: Linters/SAST tools
1 participant
Summary by CodeRabbit
New Features
load_autocommitto control autocommit for the PostgreSQL SQL loader connection..sqlfiles with statements that must not run inside a transaction (e.g.CREATE DATABASE).Documentation
Breaking Changes
autocommitargument; positional argument compatibility was affected.Tests