Normalize response types consistently when clearing responses - #747
Conversation
nevans
left a comment
There was a problem hiding this comment.
Can you add a test for this?
For future reference, the examples you've given in the PR descriptions would usually be more useful as updated or added test cases. Not everything needs a test case, but even the benchmarks could be scripts in the benchmarks directory. For this particular PR and several others, I'd've just merged it already if the examples were given as new/updated tests.
So of course, I can copy your examples myself. I probably will for several of them, because I want to merge them ASAP. But that's extra work and delay (and mental energy) when you're already generating good test cases, but putting them in the wrong place. When I have time doesn't always coincide with when I have access to my computer. For example, right now I'm writing on my phone. I can code on my phone (thanks to termux, I've done a lot of net-imap development on my phone). But it adds an extra layer and delay.
Anyway, with all of that said: Thanks for your contributions! I'd like to cut a release with most of them ASAP (probably tomorrow or Monday).
|
An additional format note: the details in the "Verification" section of your PRs are all presented at the same level of emphasis, but they're not all of equal importance to the PR. Much of what's in it, I'd classify as debug logs (or bot slop, if I'm being uncharitable). For bot generated PRs, I do want to see more detailed logs. They are a checklist that basic due diligence was done. But they are of minimal relevance to the PR itself. I'd strongly recommend putting those "debug log" details behind a disclosure. That way, the verification that're specifically relevant to this PR can be called out. For example: ### Verification
- 90 focused checks, 20 failing expectations before and zero afterward.
- Five string/symbol case variants, empty/nonempty response lists, actual key deletion, unrelated-response retention, frozen output and repeated clearing are covered. The original #93 introduced both methods; its source was reviewed.
<details>
<summary>Detailed verification log</summary>
- Existing `rake test` on this isolated branch: **1726 tests, 12602 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications**, Ruby 4.0.6 via rbenv. Baseline also passes 1,726 tests; assertion counts vary slightly between runs.
- Supplemental RuboCop Lint retains the same 48 existing findings. Syntax and git diff --check pass. No new/modified repository tests, dependencies or workflows; focused checks were external under the consumer repository's no-new-tests policy.
- Based on master `6d2ef7a636a1e2449187a83b06ac7a5baa54ead2`; runtime differs from released 0.6.6 only in documentation before this change. Existing upstream PR searches found no matching fix.
</details>which renders as: Verification
Detailed verification log
Thanks. |
Summary
Normalize response types with to_s.upcase in clear_responses, matching responses(type) and extract_responses(type). Lowercase/mixed-case strings and symbols can currently read a response but silently fail to remove it.
Reproduction
Verification
rake teston this isolated branch: 1726 tests, 12602 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications, Ruby 4.0.6 via rbenv. Baseline also passes 1,726 tests; assertion counts vary slightly between runs.6d2ef7a636a1e2449187a83b06ac7a5baa54ead2; runtime differs from released 0.6.6 only in documentation before this change. Existing upstream PR searches found no matching fix.Compatibility and limits
Intentional correction: noncanonical type spellings now clear the corresponding canonical response key. Uppercase strings, nil/false whole-hash clearing and result freezing retain their existing behavior. This does not alter raw response-hash mutation policy. No production or external IMAP service used. Other Ruby/OS versions were not run locally. Local success does not imply upstream CI approval or exhaustive coverage.