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 what happened next — the review, the follow-up PR, and the merge.

The first review

A review comment arrived the next day, and it was thorough: why the fix logic is correct, whether the tests match the repo's existing test conventions, even whether every API the new test uses actually exists on the base branch. The verdict was approve, and the PR was merged seconds after the review comment appeared. It did come with suggestions, though:

  • 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 the follow-up, #1268, the same day #1267 was merged. It took up the suggestions in three parts: extracting the check into a shared Strings.containsNonAsciiWhitespace, applying it to IdnEmailFormat, and strengthening 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. And of those 18, only three — U+00A0, U+2007, U+202F — slip past isWhitespace and are caught only by isSpaceChar. There was no need to test all 28, so 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, including Korean ones, produce for a full-width space).

The license header suggestion I followed halfway. IdnEmailFormat had no header at all, so I added one matching the existing source files; the test files, it turned out, consistently use a different header form, so I left those as they were and put the reasoning in the PR description up front.

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

The second review

Four days later came a second review, and this one didn't stop at reading. The reviewer had built the branch, run the full suite, and confirmed that deleting my fix makes the new tests fail. My tests weren't decoration — and it was the reviewer, not me, who proved it.

There were four suggestions. My first thought was that I'd need to apply all four, but read one by one they weren't at the same temperature at all:

  1. Asymmetric test coverage — EmailFormatTest generalizes over U+2003/U+3000, while IdnEmailFormatTest only tests U+00A0; mirror the same case across. Noted as "a two-line addition."
  2. Near-identical test helpers — the two classes' helpers differ only in the format name. But conditional: 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. Also conditional: "only worth doing if format validation shows up in profiling."
  4. The header year — 2016 predates the file's creation (2023). But the review itself concluded the PR's reasoning was defensible and called it "a maintainer preference call, not a defect."

The review's last line settled it: "Suggestion 1 is the only one I'd bother asking for, and it's optional." Only the first one actually mattered, so the first one is what I did.

Applying it, and the reply

The change itself was small — a 14-line commit mirroring EmailFormatTest's parameterized test (the same test run repeatedly with different inputs) into IdnEmailFormatTest.

I pushed and left a reply: what I'd applied, what I'd deliberately left alone, and that this was my first open-source contribution. For the closing line I'd wanted to use a traditional Korean well-wish — something like "may your days ahead be filled with nothing but happiness." But rendered in English, that reads like a goodbye to someone you never expect to see again. I went with "Looking forward to contributing more" instead.

Re-review and merge

Less than an hour later the re-review arrived — again verified by running, not just reading: exactly two more tests than before, matching the two new parameterized cases; exactly three failures when the check is deleted; and a table confirming that email and idn-email now behave identically on every input. The two remaining notes were both explicitly "recording, not requesting" — written down so nobody rediscovers them as bugs later.

The verdict was "approve and merge," and the PR was merged moments later. At the end of the review was this:

Also worth saying: for a first open-source contribution, splitting the narrow fix from the follow-up refactor, pre-empting the license-header question with evidence in the PR body, and building invisible characters from code points instead of pasting them into source are all things regular contributors get wrong.

It was a response to my comment mentioning this was my first contribution. To the maintainer, stevehu, who built and ran a stranger's PR to review it properly and then left words like these — thank you.