Code Review, Requirements, and Community Particiation

[Thomas Passin]

In the announcement about the proposed PR 3215 that massively affects UNLs, @Edward wrote

"I won't wait for a code review. The code involved is too tricky to understand in an hour or five."

This statement contains two red flags. If it's too tricky for a code review, there's something out of whack somewhere.  The answer isn't to skip code review, it's to figure out how to make it more accessible so that it can be commented on and reviewed.

One important aspect of that is having requirements.  If we had a proposed set of requirements for a change to fix a well-defined problem, we probably wouldn't be in the fix we're in right now about this PR.  The Leo community could have commented on the requirements and perhaps suggested changes.  True, I seem to be the only part of the community that has responded, but maybe others would have.

Then changes could have proposed and it could be explained how they would work to satisfy the requirements.  And once that all seemed all right, then tests could have ben written, the details could have been implemented and tested.

I'm not suggesting that written requirements would be needed for minor changes or bug fixes.  But for something which is too complicated to review?  And without requirements, you can't really write proper tests, either  The way it has worked in this case, the "requirements" live in one person's head, are not very well specified, and are subject to rapid change.  (I'm like that, too! But sometimes, I do write requirements just for myself.)

Yes, it might have taken longer to formulate some requirements and let the community digest and comment on them.  But there was no need for speed.  The impetus for this was not a critical bug.  There was no need to rush major, breaking changes.

If I had seen some requirements, I would have asked to add one similar to this -

Backwards Compatibility

    1. Existing UNL-consuming code shall work without changes.

    2. CTRL-Click navigation to legacy UNLs shall continue to work correctly.

    3. The status bar shall continue to display a representation of the path to the selected node.

Now, maybe item 1. wouldn't be possible.  Then we could have had a discussion about why not, and how to handle using the new UNLs with existing functions.  We could have changed that requirement to make it clear what was supposed to be accomplished.  If we had a sketch of proposed changes, we could have seen where method signatures were changed, and if that had been intended or a mistake.

All this would have made Felix's life easier, too!  And I imagine that he would have had some good input to contribute.

[jkn]

I'm not going to go too far down this rabbit hole - I have a day job - but in general I would agree that 'too tricky to understand' code is a red flag.

[Mike Hodson]

On Mon, Jun 26, 2023, 13:34 Thomas Passin <tbp1...@gmail.com> wrote:

In the announcement about the proposed PR 3215 that massively affects UNLs, @Edward wrote

"I won't wait for a code review. The code involved is too tricky to understand in an hour or five."

This statement contains two red flags.

I might be miscounting, but the entire gist of your email appears to only list the one red flag of two tricky to review.

I'm curious what you think the second red flag was?

In my opinion a red flag of 'pushing through a change without review' regardless of supporting reasoning, comes up well before the complexity of the code.  

I think it's antithetical to the open source mantra, even with a BDFL in place who gets to make the final decision on things, to simply push a change through without even asking for comment.

If it's too tricky for a code review, there's something out of whack somewhere.  The answer isn't to skip code review, it's to figure out how to make it more accessible so that it can be commented on and reviewed.

And also perhaps not assume that people would not understand the code, even if it's tricky there is a good potential that at least a few people might be able to understand what the changes imply.

Mike

[Edward K. Ream]

On Mon, Jun 26, 2023 at 2:34 PM Thomas Passin <tbp1...@gmail.com> wrote:

In the announcement about the proposed PR 3215 that massively affects UNLs, @Edward wrote

"I won't wait for a code review. The code involved is too tricky to understand in an hour or five."

I'll retract this statement. There have been many good comments already. And I'm glad I waited.

Otoh, what I think the code needs most right now is testing.

Edward

[Edward K. Ream]

On Mon, Jun 26, 2023 at 2:34 PM Thomas Passin <tbp1...@gmail.com> wrote:

If we had a proposed set of requirements for a change to fix a well-defined problem, we probably wouldn't be in the fix we're in right now about this PR.

We aren't in a fix. We just need to find the least disruptive way of transitioning to the new unls

The PR has had a life of its own, driven by exploration, not requirements.

The exploration involved simplifying the code and finding the limits of compatibility. The result has been worth two weeks of my life.

The PR won't be merged until neither you nor everyone else has any further significant objections.

Edward

[Thomas Passin]

On Monday, June 26, 2023 at 5:15:45 PM UTC-4 mys...@gmail.com wrote:

On Mon, Jun 26, 2023, 13:34 Thomas Passin <tbp1...@gmail.com> wrote:

In the announcement about the proposed PR 3215 that massively affects UNLs, @Edward wrote

"I won't wait for a code review. The code involved is too tricky to understand in an hour or five."

This statement contains two red flags.

I might be miscounting, but the entire gist of your email appears to only list the one red flag of two tricky to review.

I'm curious what you think the second red flag was?

 

1. No code review;

2. Too tricky.