Skip to content

docs(upstream): run the three open PRs through the review panel - #149

Merged
John-Cusack merged 1 commit into
mainfrom
fm/rs-upstream-audit-open-prs
Oct 29, 2026
Merged

John-Cusack merged 1 commit into
mainfrom
fm/rs-upstream-audit-open-prs

Conversation

@John-Cusack

Copy link
Copy Markdown
Owner

Ran the three open upstream PRs through the process in docs/UPSTREAM_PRS.md: why, per-hunk measurement, import time, documented settings, a differential, then three fresh reviewers per packet, up to three rounds.

Outcomes

  • Skip the IDNA round trip for domains without Punycode labels JoshData/python-email-validator#182: cut down to change 1 (+7 in syntax.py, +1 changelog line) and force-pushed to the PR branch, with a new title and body. Round 1 failed the maintainer and facts reviewers. Both caught that the IDNA round trip does not "only lowercase" the domain, because uts46_remap has already case-folded it, so .isascii()/.lower() were dead code. Round 2 passed all three. Mixed benchmark 54 → 30 µs per address. Import time, memory and all 11 documented settings are unchanged, and 60,044 differential cases are identical. The reply to "But why?" passed all three on round 3. Not posted.
  • Memoize parse-pattern builds across instances; reuse one parser per locale arrow-py/arrow#1398: held, closing recommended. Facts found a real defect: the class-level cache can return another parser's compiled pattern, after which a plain arrow.get("2013", "YYYY") raises. An 8-line factory-only version (lru_cache plus the existing cache_size) gets all of the per-call gain, but the only real caller we found (ics 0.7.3) gets less than 5%. The closing comment used all three rounds and still has one facts finding (the failure depends on order), so it goes to the person signing off.
  • Leaner generated validation code (~1.5x on object-heavy schemas), no behavior change horejsek/python-fastjsonschema#233: held, closing recommended.
    • "No behavior change" is false: {"properties": []} now compiles, and an all-string enum accepts non-string objects with a matching __eq__.
    • The body says "draft04 only", but draft06 and draft07 subclass draft04.
    • On the repo's own performance.py the gain is about 6%.
    • The closing comment passed all three reviewers on round 1. Not posted.

Nothing was posted on any upstream thread. Every packet, verdict and piece of raw evidence is kept outside the repo.

Changes

  • docs/UPSTREAM_PRS.md:
    • New retrospective section with what each reviewer flagged in every round, the per-hunk tables and the final texts.
    • A correction to the earlier #182 retrospective ("only lowercases").
    • One added rule in step 3: time nothing while reviewers run, and put every quoted number's script and output in the evidence.
  • docs/PYTHON_SPEEDUPS.md: the PR URL and status cells for email-validator, arrow and fastjsonschema.

Checks

Docs only, no crate, prompt or script changes. cargo build/test/clippy and the acceptance scripts are unaffected and were not rerun. No em dashes were added.

email-validator#182 cut to the 7-line early return and pushed after two
panel rounds; arrow#1398 and fastjsonschema#233 held with close
recommended. Records each reviewer's findings, the per-hunk numbers,
the reviewed reply and closing texts, and updates the tracker rows.
@John-Cusack
John-Cusack merged commit 8569756 into main Oct 29, 2026
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