Repository navigation
learn: retrospective learnings - #1018
peco-engineer-bot[bot] wants to merge 2 commits into
Conversation
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
peco-engineer-bot
Bot
added
the
engineer-bot-learning
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
This PR only adds to the knowledge log, and it looks good apart from one wording issue. Both new entries match the code: _renders_as_in_list excludes bytes/bytearray, and _markers_following_in swaps escaped strings for '' and checks the nonce. One nit, not posted inline: the new entries indent the **Rule:** continuation line by 4 spaces, while every earlier entry uses 2.
|
|
||
| ### 2026-11-10: learnings since 2026-11-09T17:26:03Z | ||
| - **Context:** PR #1013 (#629) made inline params render a sequence directly after `IN` as `(1,2,3)` instead of `ARRAY(1,2,3)`. The reviewer noticed that `_renders_as_array` treats any non-`str` `Sequence` as list-able, and that includes `bytes`/`bytearray`. Before the change, `x IN %s` with bytes rendered as `IN ARRAY(97,98,99)`, which Spark rejected with a loud `PARSE_SYNTAX_ERROR`. After it, the query became valid SQL and quietly matched the wrong rows. The fix left bytes out of the IN-list path. | ||
| **Rule:** When you widen which input types take a new rendering or conversion path, list the types that used to fail loudly (bytes, str, sets, empty containers) and check that none of them now produce valid but wrong output. Exclude those types on purpose and pin each one with a unit test, because a loud error that turns into a silent wrong result is a regression. |
There was a problem hiding this comment.
🔵 Low — The Rule lists sets next to bytes as types that "used to fail loudly", then says "Exclude those types on purpose". In the same PR (#1013), sets were deliberately moved onto the new path: _renders_as_in_list accepts AbstractSet, and render_inline sorts the items. Read literally, the rule tells a future author to exclude any type that used to fail loudly. That conflicts with the code this entry describes. Suggest rewording so the instruction is to check each previously-failing type and exclude it only when the new output would be wrong (as with bytes/bytearray), not to exclude all of them by reflex.
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Nit
Looks good. This is a docs-only append to the learning log, and I found only one formatting nit. I checked the factual claims against the repo: the kernel extra is ^1.4.0 in pyproject.toml:73, and kernel-e2e.yml builds from the KERNEL_REV SHA. Both match what the entries say.
Other findings
- ⚪ Nit — The
**Rule:**continuation lines in the 2026-11-10 section are indented 4 spaces. Every other entry in the log, including the 2026-11-11 section, uses 2. It renders the same, but it's worth normalizing to 2 for consistency.
Rolling retrospective learnings
This PR accumulates one dated section of learnings per day (from merged PRs and engineer-bot author runs) until it is merged; merging it starts a fresh one. The bot never writes the canonical log directly.
Latest update 2026-11-11: 2 new learning(s) since 2026-11-10T17:25:56Z.