DEV Community

VINAYAK GOTE
VINAYAK GOTE

Posted on

How I Extended a Refaster Rule in Picnic's error-prone-support (and Recovered a PR GitHub Broke)

#opensource #java #codequality #testing

This is post 4 in my open source series. After Zalando Logbook and the Adyen Java library, this one is about Picnic's error-prone-support, and it has a twist: the PR I opened got stuck in a broken state on GitHub, and I had to recreate it.

What is error-prone-support?

error-prone-support is an open source project from Picnic. It extends Google's Error Prone with extra bug checks and Refaster rules. A Refaster rule says "this code pattern should be rewritten like this", and it can fix it automatically across a codebase.

What I worked on

The project already had a rule called RequireNonNullExpression. I extended it to cover this pattern:

// Before: Optional.of() throws a NullPointerException if value is null,
// so the Optional can never be empty. The orElse() part is dead code.
String result = Optional.of(value).orElse(fallback);

// After: says exactly what the code really does
String result = requireNonNull(value);
Enter fullscreen mode Exit fullscreen mode

The idea is simple. Optional.of(x) can never be empty, so wrapping and then calling orElse only hides what is happening. The rewritten version is shorter and honest about the null check.

It closed issue #436.

The twist: GitHub broke my PR

My first PR, #2264, got into a strange state after an accidental close. The page showed "Merge status cannot be loaded", and the Reopen button was disabled.

I contacted GitHub Support. They confirmed it is a known problem with no manual fix. The only way out was to create a new PR from the same branch.

So I opened #2275 with the exact same commits, plus the Javadoc wording a maintainer had suggested, and linked it back to the original. The review carried on from there.

What the review taught me

  • Do not duplicate what already exists. One maintainer pointed out that an existing check, OptionalOfRedundantMethod, already does something similar. I looked into the source and explained the difference in intent: its fix assumes the value might be null, while this rule targets values that are provably non-null. In the end, the cross-reference comment was dropped.
  • Test your assumptions, don't just argue them. I assumed Refaster rules chain together on one input. The maintainer explained that the tests do a single pass and users are expected to apply rules until nothing changes. I ran the full test suite (78 tests, 0 failures) to confirm my expected output was correct as it was.
  • Small style rules matter. The reviewers asked me to use @code instead of @link in Javadoc when a link would pull in a new dependency or import. It is a tiny change, but it follows how the project keeps its code clean.
  • Patience is part of the job. The review went slowly because of holidays and busy schedules. A maintainer even said they had not forgotten the PR. Waiting politely and staying responsive worked better than pushing.

The PR was approved and merged by the maintainers.

What I would tell someone starting out

  1. If GitHub or CI breaks your PR, don't panic. Recreate it from the same branch and link to the original.
  2. When a reviewer questions your approach, check the source and run a test before you answer. Answers backed by evidence end discussions faster.
  3. Learn the project's conventions (comments, Javadoc style, test layout) by reading existing code before you write yours.
  4. Thank reviewers. They are spending their time on your work.

Proof of my work

Everything here is public, so you can check it yourself:

Next post: a PR of mine that did not go through, why the maintainers said no, and what I took from it.

If you are a maintainer, a recruiter or a team looking for someone who learns in public and likes to collaborate, my DMs are open. And if you have ever had a PR stuck in a weird state, tell me in the comments how you fixed it.

Top comments (0)