Re: Code Review, Requirements, and Community Particiation
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 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. -- 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 leo-editor+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/leo-editor/26635273-bff9-45cf-96d4-986ab5f91b8en%40googlegroups.com.
Re: Code Review, Requirements, and Community Particiation
On Mon, Jun 26, 2023 at 2:34 PM Thomas Passin 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 -- 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 leo-editor+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/leo-editor/CAMF8tS2Je9at93AJt%3DMn2SHEr%2Bw0TaOh1FH3ypqK0-P2n_iPHw%40mail.gmail.com.
Re: Code Review, Requirements, and Community Particiation
On Mon, Jun 26, 2023 at 2:34 PM Thomas Passin 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 -- 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 leo-editor+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/leo-editor/CAMF8tS32nZx3SkGidyagHFzw_SMf5EQSjxKoZKB4QT7vXnWjGQ%40mail.gmail.com.
Re: Code Review, Requirements, and Community Particiation
On Mon, Jun 26, 2023, 13:34 Thomas Passin 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 -- 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 leo-editor+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/leo-editor/CAN%2B%2B4hHQMw0YC%2BGtc3Xa_%3DmVM833Szd59tMpL3KbHuK4V_g0eA%40mail.gmail.com.
Re: Code Review, Requirements, and Community Particiation
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 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. 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 leo-editor+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/leo-editor/ab80e178-958f-4400-a5ab-090b3de90ccdn%40googlegroups.com.
Code Review, Requirements, and Community Particiation
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 leo-editor+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/leo-editor/2eaa6d8b-c6aa-43d6-91f7-3bd25459575cn%40googlegroups.com.
