druid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gian Merlino <g...@apache.org>
Subject Code reviews, UX, and tests
Date Thu, 15 Oct 2020 09:21:28 GMT
Hey Druids,

I am writing to you all to ask for your help 🙂

In particular, your help in ensuring that potential code contributions are
reviewed in a timely fashion. Right now we have 72 open PRs, which due to
stalebot are mostly opened pretty recently. That's a lot of people that
want to contribute stuff!

First, I wanted to point out that a lot of the reviews are done by a
relatively small number of people. If you are one of those people, thank
you. If you aren't, please consider becoming one.

Second, I've been thinking about how to make reviewing a little easier.
Maybe this will help more people become reviewers. I'd like to suggest
focusing on user experience and tests.

1) "User experience" means any new properties being introduced, any new
features being introduced, any behavior changes to existing features.

2) "Tests" are ideally automated tests that run as part of Travis CI. But
there are other kinds of tests too.

It's important to get user experience right, because if we release a
feature with poor UX, then it's hard to change later without breaking
compatibility. We'd like the master branch to be releasable at all times,
which means we should figure this stuff out before committing a patch.

It's also important to get tests right, because they ensure high quality
releases, and they also ensure that future changes can be made with low

It's less important to get other code-level things right, because we can
always change them later, and if there aren't UX or quality impacts then no
harm was done.

For contributors, focusing on UX and tests means writing out (in natural
language) how your patch changes user experience, and why you think this
change is a good idea. It also means having good testing of the new stuff
you're adding, and writing out (in natural language) why you think your
tests cover all the important cases. Speaking as a person that has reviewed
a lot of code: these natural language descriptions are *very helpful*,
especially when they add context to the patch. Don't make reviewers
reverse-engineer your code to guess what you were thinking.

For reviewers, it means that if we are confident in the UX and testing, we
don't need to spend a ton of time poring over every line of code. (I'd
still take the time to review key classes and interfaces, but this doesn't
take as much time as reviewing every line.) It also means that if we get a
PR that isn't set up to enable quick review, then we should write that to
the contributor and invite them to improve their PR, rather than ignoring
it or spending too much time on it. (Of course, we should only ask for this
if we can actually follow up with a review when the PR is improved later

I'd love to hear what people think.


  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message