Allowing comments after the line continuation backslash

[Mark Wooding]

Lawrence D'Oliveiro <ldo at geek-central.gen.new_zealand> writes:

> In message <878w12kt5x.fsf.mdw at metalzone.distorted.org.uk>, Mark Wooding 
> wrote:
> >   2. The MainWindow class only has the `Window' attribute described in
> >      its definition.  Apparently there are other attributes as well (the
> >      ColorMumbleList family for one, also `LayerDisplay').  Similarly,
> >      the LoadedImage class has attributes `Contents' and `FileName', but
> >      I see attributes `RowStride', `ImageDrawSize' and `SrcDimensions'
> >      used immediately below.
>
> These are really just namespaces, one for the GUI context and the other for 
> the image data.

I think my point was about the rather selective documentation of the
namespaces.  The trivial-class trick is something I use myself (though
usually to simulate a structure rather than just to carve up namespace)
so I don't find it objectionable in and of itself.

> >   3. In SelectSaveMenu, there's a curious `for' loop:
> > 
> >         for State in (gtk.STATE_NORMAL,) : # no need for
> >         gtk.STATE_INSENSITIVE
> >           ## blah
> > 
> >      Firstly, this is obviously equivalent to `state = gtk.STATE_NORMAL'
> >      followed by the loop body (YAGNI).
>
> It’s a tuple of one element. It used to be a tuple of two, and there is the 
> possibility it might need to become that again (as intimated at in the 
> attached comment). That’s why it stays a tuple.

I guessed the history.  I'm in two minds about this sort of thing.  I
/like/ history: I'm fascinated by the history of old programs and
programming languages; but looping over a single thing just seems weird.

> >      Of course, being boilerplate, the similarities visually outweigh the
> >      differences, when in fact it's the differences that are
> >      interesting.
>
> That is generally how code reuse works. All the “boilerplate” is in the 
> function definition, so I just have to call it parameterized by the 
> differences appropriate to each instance.

Except that's not the case.  There are extremely close similarities
between the six calls which strongly suggest (to me, anyway) that
further factoring would be beneficial.  I detest writing anything more
than once, and don't much enjoy playing spot-the-difference when reading
code.  The six calls are a screenful of spot-the-difference.

> >      It doesn't help that all of the names of the things involved are so
> >      wretchedly verbose.
>
> Oh please, no. Since when are explanatory names “wretchedly verbose”?

When the start and end bits are the same and the different bits are
hidden in the middle.  After staring at a screenful of
MainWindow.MumbleDisplay my head starts spinning.

> >      How about the following?
> > 
> >         def make_listview_l1 ...
>
> And what, pray tell, is the significance of the name “make_listview_l1”? If 
> there is something I would describe as “wretched”, it is the use of random 
> numerical suffixes like this.

It's a terse reference to List1Box (your name).  Since it was only being
used in the following two lines, I figured the name didn't matter much.
(In Lisp, I'd have wrapped the two lines in FLET and simply called the
function FROB.)

> >     Looking at some of the rest of the code, it might (or might not) be
> >     worthwhile actually making a class to gather together a list's model
> >     and view; subclasses can then vary their individual parameters, and
> >     the various colour lists can also keep the various captions with
> >     them.
>
> Most of that variation is already handled without the limitations of 
> thinking in classes. For example, selection of a colour in all three lists 
> is handled through a single EditColor routine. And of course you’ve already 
> seen how a single DefineScrollingList routine can be used to set up all the 
> scrolling lists used in this GUI.

There's so much commonality in the arguments that I'm not convinced that
they're fully factored.  Worse, there are several tables involving your
various ColorMumbleLists: adding another would require fiddling with all
of them, which suggests that things have been sliced up the wrong way.
(I'm aware in this instance that the set of such things is externally
constrained in this case.)  Maybe packaging all of the information about
each individual list in one object and having a list of these objects
would be better.

> >   5. Much of the indentation and layout is rather unconventional, though
> >      not intolerable.  But I found (deep in `SelectSaveMenu'):
> > 
> >         NewRenderPixels = array.array \
> >           (
> >                 "B",
> >                         FillColor
> >                 *
> >                         (ImageRowStride // 4 * ImageBounds[1])
> >           )
> > 
> >       to be most disconcerting.
>
> Is the structure of the expression not apparent to you?

No, it really isn't.  Oddly enough, I'd find

        (setf new-render-pixels
              (array:array "B"
                           (* fill-color
                              (floor image-row-stride 4)
                              (aref image-bounds 1))))

much easier to cope with.

> You’d probably need plenty of fresh air after something like this,
> then:
>
>         PatternArray = array.array \
>           (
>             "I",
>                 16 * (16 * (Light,) + 16 * (Dark,))
>             +
>                 16 * (16 * (Dark,) + 16 * (Light,))
>           )

That's not noticeably worse.  The bizarre thing is the relative
indentation of the operands to the * -- the oddness of which is
emphasized by the initial argument.

> >       Also, I couldn't work out why some parens are indented only two
> >       spaces and others are indented the full eight.
>
> Eight?? You mean four.

No, eight.  Text editors are misleading: tab stops for plain text files
are not a matter of individual preference.  (I read the code straight
off GitHub: my browser correctly interpreted tabs as moving to the next
multiple of eight columns.)

-- [mdw]