openlp-core team mailing list archive

[jerryb <gerald.britton@xxxxxxxxx>]

> Gerald, please can you just conform to our at times convoluted and weird
> standards. It's better having weird ones that none at all.

Raoul, one could say that it's better being mostly dead than all dead (think Miracle Max in The Princess Bride), but that's not saying much.  In this context, weird standards are better than none, but not by much.  

I'm afraid that I'm likely to be a bit of a squeaky wheel but I cannot help but speak up when I see departures from industry best practices.  Note that indentation standards are not unique to Python.  The ones I use are widely followed in C, Java, SQL, HTML/XML, Perl, ... even FORTRAN and COBOL.  It is hard for me to intentionally go against good form and break good habits.

In this case, the recommendation in the coding standards is incomplete in that it does not explicitly address line breaks inside parenthesized expressions.  

Note: Not sure if my indentation will be respected!

The example given:

1.
self.addToolbarButton(translate('SongMediaItem', 'New Song'),
    translate('SongMediaItem', 'Add a new song'),
    ':/songs/song_new.png', self.onSongNewClick, 'SongNewItem')

would look better like this:

2.
self.addToolbarButton(
    translate('SongMediaItem', 'New Song'),
    translate('SongMediaItem', 'Add a new song'),
    ':/songs/song_new.png', self.onSongNewClick, 'SongNewItem')

but even then does not address my issue.  If 'Add a new song' were replaced by some very long phrase, following the current standard would yield:

3.
self.addToolbarButton(translate('SongMediaItem', 'New Song'),
    translate('SongMediaItem', 
    'Add a new song which has an extraordinarily long title that just won't fit'),
    ':/songs/song_new.png', self.onSongNewClick, 'SongNewItem')

but then it appears that the phrase is an argument to self.addToolbarButton and not translate.  However, with proper indenting we get:

4.
self.addToolbarButton(
    translate('SongMediaItem', 'New Song'),
    translate('SongMediaItem', 
        'Add a new song which has an extraordinarily long title that just won't fit'),
    ':/songs/song_new.png', self.onSongNewClick, 'SongNewItem')

At once, it is clear that the long phrase belongs to translate() and not the superior function.  Another way, equally valid and just as readable is:

5.
self.addToolbarButton(
    translate('SongMediaItem', 'New Song'),
    translate(
        'SongMediaItem', 
        'Add a new song which has an extraordinarily long title that just won't fit'),
    ':/songs/song_new.png', self.onSongNewClick, 'SongNewItem')

This, by the way is in the direction of the standards used at Guido's employer, Google.  Going all the way to that standard produces:

6.
self.addToolbarButton(
    translate('SongMediaItem', 'New Song'),
    translate(
        'SongMediaItem', 
        'Add a new song which has an extraordinarily long title that just won't fit'),
    ':/songs/song_new.png', 
    self.onSongNewClick, 
    'SongNewItem')

which has much to commend it.  It is easy to read the args to all the function calls, including the first one.

I strongly recommend that the project move to one of the forms shown in 4, 5, and 6, above.  More formally, "If you are continuing a line inside an expression, the continued line should be indented relative to that expression." In the meantime, I will "uglify" my patch (though I'm beginning to wonder if it should be an anonymous contribution.:-} )
-- 
https://code.launchpad.net/~gerald-britton/openlp/newbugs/+merge/62532
Your team OpenLP Core is subscribed to branch lp:openlp.