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.
On Monday, June 26, 2023 at 8:34:47 PM UTC+1 [email protected] 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. 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. > -- You received this message because you are subscribed to the Google Groups "leo-editor" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/leo-editor/ab80e178-958f-4400-a5ab-090b3de90ccdn%40googlegroups.com.
