zsh-workers
 help / color / mirror / code / Atom feed
From: Oliver Kiddle <okiddle@yahoo.co.uk>
To: Joey Pabalinas <joeypabalinas@gmail.com>
Cc: "zsh-workers@zsh.org" <zsh-workers@zsh.org>,
	Taylor West <KrokodileGlue@outlook.com>
Subject: Re: [RFC] Looking for opinions on accepting refactoring patches
Date: Sat, 17 Mar 2018 12:27:29 +0100	[thread overview]
Message-ID: <7323.1521286049@thecus> (raw)
In-Reply-To: <20180316203930.f6mikhp7iiltgmpb@gmail.com>

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


  reply	other threads:[~2018-03-17 22:10 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-16 20:39 Joey Pabalinas
2018-03-17 11:27 ` Oliver Kiddle [this message]
2018-03-17 23:17   ` Joey Pabalinas
2018-03-17 23:39     ` Bart Schaefer
2018-03-18  1:48       ` Joey Pabalinas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7323.1521286049@thecus \
    --to=okiddle@yahoo.co.uk \
    --cc=KrokodileGlue@outlook.com \
    --cc=joeypabalinas@gmail.com \
    --cc=zsh-workers@zsh.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).