From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13690 invoked by alias); 17 Mar 2018 22:10:34 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: List-Unsubscribe: X-Seq: 42474 Received: (qmail 9902 invoked by uid 1010); 17 Mar 2018 22:10:34 -0000 X-Qmail-Scanner-Diagnostics: from park01.gkg.net by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.99.2/21882. spamassassin: 3.4.1. Clear:RC:0(205.235.26.22):SA:0(0.1/5.0):. Processed in 1.83982 secs); 17 Mar 2018 22:10:34 -0000 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=0.1 required=5.0 tests=BAYES_00,DATE_IN_PAST_06_12, FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS, RCVD_IN_DNSWL_NONE,SPF_PASS,T_DKIM_INVALID,T_RP_MATCHES_RCVD autolearn=no autolearn_force=no version=3.4.1 X-Envelope-From: SRS0=W53b=GH=yahoo.co.uk=okiddle@bounces.park01.gkg.net X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | X-Virus-Scanned: by amavisd-new at gkg.net Authentication-Results: amavisd4.gkg.net (amavisd-new); dkim=pass (2048-bit key) header.d=yahoo.co.uk X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.co.uk; s=s2048; t=1521324624; bh=yDN+96F0h2utqc9VhxDsYwhsjjxovmO7QqBQUC8TLd8=; h=From:References:To:Subject:Date:From:Subject; b=uUdoAJ9PpCD3NRs3pwQwIY0HRsSJbadpyEKYAd2Z7KPPWuEk0cmYO7qCYKho4cw6UuLldgMAG5owvryRhOwAuHrpnr/B3nr3OIZWZJ1QaLXg+WOfL9YSYjaAtWs/eu8DBt7x04e+6zXxrlnhnStsJFiepchiSCAStYbgPW90Kv63tQ49MIB19erOowwSwgbG0lL+g5AKidnPR0SGiPPJMx4GEPUTyqg1g3vndkaJ0GR8Ma+BOhIyLWD/lQuFTYK+5s1s5SehQ4Cnnp1wg7WY2YhwGluxYSFix7nHruvPhTUdO7PRijTGgIyEeyXpJUS9lg27RHI3VrLTKgSygblOUQ== X-YMail-OSG: aCxYOCMVM1kmcObBF5I5p11J8_EB7D2dYfJkVlpAcknSdg1IRENpcCadOU7Kqu8 T.0nGQraJa.aEq778cDpNuaR7GiIc.rI_wFPtZgh5qHeTOC_0qc36_mGVyttM26BDEbrR4rATHo. NhTHgCWRwy2rNL53xNg9JakFJqV.WMVFeD5bqUyIB1w9fAaNkCGGA5HCv3d.QrkH71FzZWffuoxG 0VFQanyfVJ0N6oHR3ucw18kK3qMKGRCh52GTihXtGwEocyxZRqBY_019RH0EXfa2Y7XCDjn3vapD U99jPyBpSUcuDR7iK9cFSYOJpfZr2aVn1mgKVsYHl9OWsAvcQZeiKK6a3WBgkjKDdqxC9J8WX1Ur ceAAMsl6f9PHHFOVCGDayZOnYpC9sssFt9JHaLJ3iDl6JuUuQ6uFAraZhKfF5l44aeci5lPXioLA S2Jh0Jb.XEy9ErpqWCL3R0U9E4Lstcfv3uL4_1olrWKS1nYwdIxQ3yFXeX3Mz0BVFT_x2RItJxVs DBB0e5jtcuQ-- cc: "zsh-workers@zsh.org" , Taylor West In-reply-to: <20180316203930.f6mikhp7iiltgmpb@gmail.com> From: Oliver Kiddle References: <20180316203930.f6mikhp7iiltgmpb@gmail.com> To: Joey Pabalinas Subject: Re: [RFC] Looking for opinions on accepting refactoring patches MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <7322.1521286049.1@thecus> Date: Sat, 17 Mar 2018 12:27:29 +0100 Message-ID: <7323.1521286049@thecus> Joey Pabalinas wrote: > The reason is that many parts of the Zsh codebase are literally > _byzantine_labyrinths_ of single-letter identifiers and uncommented > shenanigans that were very confusing to figure out. > But in my opinion this is probably why I haven't noticed as many people > contributing to Zsh as I expect to (it really is an absolutely *amazing* There may be other reasons but I'm sure it hasn't helped. > piece of software engineering in my opinion), so I was interested in > some comments about how patches concentrated only on refactoring the > code with semantics changes whatsoever would be received? I'm sure any improvements would be welcomed. Notions on good refactorings can be subjective; such as breaking a long function into smaller ones where the original long function was neatly divided into self-contained blocks anyway. So as always, it depends on the particulars of the patch. > Is this something that would be encouraged? Or is the risk of bugs > and regression just too heavy for this to be a realistic goal? The best way to alleviate the risk of bugs is to add test cases at the same time. If you're going through to make sense of the code, test cases will occur to you naturally anyway. Running the existing tests with code coverage enabled helps to see if code is getting any existing testing. And if you're willing to fix what you break then I can't see that anyone can have any complaints: When I went through much of the guts of comparguments last year my main aim was to fix a few bugs that had been bothering me for years rather than refactoring. I'm usually reluctant to make quick and dirty fixes without fully understanding the code. In that case, I accepted that I would need to fix whatever I broke. That code might still be further refactored but it is cleaner than it was. But, having added test cases while going through to make sense of the logic, I'm now in a position where I'm fairly confident that I can dabble in that part of the code without breaking things. It may also help if, when choosing what code to refactor, you have a longer term view to some bugs you'd like to see squished or even features that might be added. Oliver