Getting reviewed — my first open-source contribution (part 2)

no.2 · 2026-07-29

PR #1267 review → PR #1268

Part 1 covered picking the issue, finding the cause, fixing it, and submitting the PR. Part 2 is the review, the follow-up PR, and what I left undone.

The first review

The review arrived the next day and the PR was approved and merged. It came with three suggestions:

  • Strengthen the tests: assert that a quoted local part ("joe bloggs"@example.com) still passes, and generalize beyond U+00A0
  • idn-email has the same bug — if there's a follow-up, extract the check into a shared helper
  • Align the license header with the existing files

The sentence I'd left in the PR description — "idn-email is out of scope" — had just become the spec for the next PR.

The follow-up PR

I opened #1268 the same day: extract the check into a shared Strings.containsNonAsciiWhitespace, apply it to IdnEmailFormat, and strengthen the tests on both sides.

idn-email needed one extra bit of care. The format's entire reason to exist is allowing non-ASCII letters — münchen@example.com — so a whitespace check that also blocked letters would break the format itself. I added a test asserting that a letter like U+00FC (ü) still passes.

Choosing which characters to generalize the tests over needed a basis too. Running Java's two predicates across all of Unicode, exactly 28 characters classify as whitespace — 10 in the ASCII range, 18 beyond it. Of those 18, only three — U+00A0, U+2007, U+202F — slip past isWhitespace and are caught only by isSpaceChar. Rather than test all 28, I picked representatives from groups that take the same code path: U+2003 (em space) and U+3000 (ideographic space — the character CJK input methods produce for a full-width space).

IdnEmailFormat had no license header, so I added one matching the existing source files. The test files use a different form, which I left as it was and noted in the PR description.

All told: 5 files, +146/−18.

The second review

The second review came four days later. It didn't stop at reading the diff — the reviewer built the branch, ran the suite, and confirmed that deleting the fix makes the new tests fail. That's what a review is supposed to do, and it's worth naming because it's the part that actually establishes whether tests test anything.

Four notes came back:

  1. Asymmetric test coverage — EmailFormatTest generalizes over U+2003/U+3000 while IdnEmailFormatTest only tests U+00A0. Mirror the case across.
  2. Near-identical test helpers — the two classes' helpers differ only in the format name. Consolidate if a third format ever needs the same guard.
  3. A micro-optimization — an indexed loop instead of a stream would avoid an allocation, if format validation ever shows up in profiling.
  4. The header year — 2016 predates the file's creation.

I applied the first.

Applying it, and the merge

The change was a 14-line commit mirroring EmailFormatTest's parameterized test into IdnEmailFormatTest. I pushed and replied with what I'd applied and what I'd left alone.

The re-review was again verified by running rather than reading: two more tests than before, matching the two new parameterized cases; three failures when the check is deleted; and a table confirming that email and idn-email now behave identically on every input. Two remaining notes were marked as recorded rather than requested, so that nobody rediscovers them as bugs later. Approved and merged.

The fix itself was five lines. Almost everything I learned came from the two rounds of review around it.