[RFC] Looking for opinions on accepting refactoring patches

[RFC] Looking for opinions on accepting refactoring patches

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 2400 bytes --]

Recently I've become interesting in contributing to Zsh, and
have already been submitting a few scattered patches here and
there, but I will admit it's been a pretty trying experience
overall.

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.

It seems I'm not the only one with difficulties understanding many
parts of the code, as a quick `git grep` for things like 'why' '??'
and 'what' reveal gems like:

> Src/Zle/computil.c:2850: * -->PLEASE DON'T ASK<--

> Src/Zle/complist.c:3431: * *** PLEASE DON'T ASK ME WHY THIS IS NECESSARY ***

> Test/E01options.ztst:879: # Goodness only knows why.

> Src/glob.c:2488: /* please do not laugh at this code. */

> Src/params.c:4167: /* ??? error checking */

> Src/hist.c:3196: * However, I'm so confused it could simply be baking Bakewell tarts.

> Src/Zle/computil.c-4240-
>
> * As there are virtually no comments in this file, I don't really
> * know why we're doing this, but it's to do with a matcher which
> * is passed as an argument to the utility compfiles -p/-P.

> Src/Zle/zle_tricky.c-1080-
>
> /* Lasciate ogni speranza.
> This function is a nightmare.  It works, but I'm sure that nobody really *
> understands why.  The problem is: to make it cleaner we would need       *
> changes in the lexer code (and then in the parser, and then...).         */

Most of the offending code is part of the completion subsystem, and
obviously being one of the central parts of Zsh, I can understand the
reluctance on changing anything but what is absolutely needed to
to avoid breaking things.

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*
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?

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?

Quite honestly I find either answers to be an understandable position,
thus I figured it would be better to ask about it before attempting any
patches like this.

-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC] Looking for opinions on accepting refactoring patches

[Oliver Kiddle]

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

Re: [RFC] Looking for opinions on accepting refactoring patches

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 1586 bytes --]

On Sat, Mar 17, 2018 at 12:27:29PM +0100, Oliver Kiddle wrote:
> 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.

Very true.

> 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:

Alright, I agree that sounds like best way to manage regression risk. I
most certainly am; it is very depressing when people break things but
force others to fix them.

> 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.

Very good point as well. This is not something that will be undertaken
lightly; I would have to spend a _LOT_ of time reading the code before
any sort of refactoring is something I would feel comfortable
attempting.

Surgical changes very rarely work for things like this in my opinion;
without the big picture it is very easy to make things worse.

Appreciate the comments; keep it coming guys, thanks.

-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC] Looking for opinions on accepting refactoring patches

[Bart Schaefer]

To what Oliver said, I would just add:  I'm much happier accepting
patches that stay somewhat close to the code formatting style that's
already in use, which is pretty lax in general but boils down to:
- don't use // comment to end of line
- don't use CamelCaps except for typedef names
- indent 4 spaces with 8-space tab stops if you use tabs at all
- stick to 80 columns where possible (yes, we're old fogeys)
- use whitespace after keywords and commas and around open/close
braces, but not after open or before close parens and brackets
- violate that latter for readability, especially with extra line breaks

Re: [RFC] Looking for opinions on accepting refactoring patches

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

On Sat, Mar 17, 2018 at 04:39:08PM -0700, Bart Schaefer wrote:
> To what Oliver said, I would just add:  I'm much happier accepting
> patches that stay somewhat close to the code formatting style that's
> already in use, which is pretty lax in general but boils down to:

This list helps a lot, thanks!

-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]