Fix dropped PL/pgSQL EXCEPTION handlers and RAISE USING options#36
Conversation
- Iterate each proc_exception node in format_exception_sect so the WHEN conditions and handler body are rendered instead of a bare EXCEPTION keyword. The grammar wraps each handler in proc_exception, so scanning exception_sect's direct children for proc_conditions/proc_sect never matched (#26). - Preserve RAISE ... USING options in format_stmt_raise by handling the kw_using keyword and each raise_option (ERRCODE, MESSAGE, ...), which were previously discarded. Supports multiple comma-separated options (#32). - Add inline regression tests to tests/plpgsql_test.rs for both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTwo PL/pgSQL formatting bugs are fixed in ChangesPL/pgSQL formatter fixes
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)Not applicable — changes are localized formatter logic fixes without multi-component interaction flows. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/plpgsql_test.rs (1)
93-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for
:=and otherraise_optionkeywords.This test only exercises
=withERRCODE/MESSAGE. Given the grammar also allows:=and other options (DETAIL,HINT,COLUMN,CONSTRAINT, etc.), an additional case would help catch the operator-normalization concern raised insrc/formatter/plpgsql.rs.🤖 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/plpgsql_test.rs` around lines 93 - 104, The current `raise_using_options_preserved` test only covers `RAISE ... USING` with `=` for `ERRCODE` and `MESSAGE`; add an additional assertion in `tests/plpgsql_test.rs` that exercises `:=` and other `raise_option` keywords such as `DETAIL`, `HINT`, `COLUMN`, or `CONSTRAINT` so the `format_plpgsql` path continues preserving all supported option forms and operator normalization in `src/formatter/plpgsql.rs` is covered.src/formatter/plpgsql.rs (1)
390-402: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting
raise_optionformatting into a helper.The nested loop building
opt_kw/opt_exprinline adds noticeable branching to an already largematchinformat_stmt_raise. A small helper (e.g.,fn format_raise_option(&self, node: Node<'a>) -> String) would keep the match arm terse and make the option-formatting logic independently testable.🤖 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 `@src/formatter/plpgsql.rs` around lines 390 - 402, The raise_option formatting logic inside format_stmt_raise is too much inline branching for the match arm. Extract that block into a dedicated helper such as format_raise_option that takes the child node, walks named children, builds the keyword/expression pair, and returns the formatted string; then call that helper from the raise_option match branch to keep the match terse and make the logic easier to test and maintain.
🤖 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.
Nitpick comments:
In `@src/formatter/plpgsql.rs`:
- Around line 390-402: The raise_option formatting logic inside
format_stmt_raise is too much inline branching for the match arm. Extract that
block into a dedicated helper such as format_raise_option that takes the child
node, walks named children, builds the keyword/expression pair, and returns the
formatted string; then call that helper from the raise_option match branch to
keep the match terse and make the logic easier to test and maintain.
In `@tests/plpgsql_test.rs`:
- Around line 93-104: The current `raise_using_options_preserved` test only
covers `RAISE ... USING` with `=` for `ERRCODE` and `MESSAGE`; add an additional
assertion in `tests/plpgsql_test.rs` that exercises `:=` and other
`raise_option` keywords such as `DETAIL`, `HINT`, `COLUMN`, or `CONSTRAINT` so
the `format_plpgsql` path continues preserving all supported option forms and
operator normalization in `src/formatter/plpgsql.rs` is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 50b64be8-f7a7-4df1-8d48-3b42d602eacc
📒 Files selected for processing (2)
src/formatter/plpgsql.rstests/plpgsql_test.rs
- Extract raise_option formatting into a format_raise_option helper to keep the format_stmt_raise match arm terse. - Add regression test covering DETAIL/HINT USING options alongside ERRCODE. (Skipped the suggested `:=` case: it is not valid PostgreSQL RAISE USING syntax and the grammar does not parse it.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
Fixes two PL/pgSQL formatting bugs where constructs were silently dropped from the output.
Problem
EXCEPTIONkeyword with theWHEN ... THENcondition and handler body discarded. The grammar wraps each handler in aproc_exceptionnode, butformat_exception_sectscannedexception_sect's direct named children forproc_conditions/proc_sect, which never matched.RAISE ... USING ...options (ERRCODE,MESSAGE, etc.) were dropped.format_stmt_raiseonly matchedraise_level/string_literal/sql_expression, ignoringkw_usingand eachraise_option.Solution
format_exception_sectnow iterates into eachproc_exceptionnode and renders itsproc_conditionsandproc_sect.format_stmt_raisenow handleskw_usingand eachraise_option(keyword=expression), supporting multiple comma-separated options.tests/plpgsql_test.rsfor both cases.just check(fmt, clippy-D warnings, tests) passes.Fixes #26
Fixes #32
🤖 Generated with Claude Code
Summary by CodeRabbit
RAISEstatements soUSINGoptions are preserved and displayed consistently.EXCEPTIONhandlers to keep the handler body intact and correctly placeWHEN ... THENblocks.RAISE ... USINGoption preservation.