Bug#672185: RFS: gnome-session-shutdown/1.81-1 [ITP] -- Shutdown command for GNOME

[Jacopo Lorenzetti <jacopol@cyan.xubiq.com>]

On 04/06/2012 15:58, Jakub Wilk wrote:
> (I don't intend to sponsor this package.)

Thank you anyway for your useful review.

> I'd use "=" instead of ":=" in debian/rules, so that the variable is not
> uselessly evaluated even when it's not used. (And it won't be used most
> of the time.)

Fixed.

> The definition of DEBIAN_DIR could be simplified to:
> 
> $(dir $(firstword ${MAKEFILE_LIST}))

Fixed.

> That said, I wouldn't bother with implementing get-orig-source target
> when pristine upstream tarballs is being used.

Dropped.

> What is build-dependency on docbook2x for?

Removed. It was useless.

> I see some bugs and weirdnesses in upstream code:

Being myself upstream, I fixed the bugs and released a new version.

> string module is so 90s! ;) Most of functions from that module
> (including the ones you use) are deprecated.

Replaced string functions with string object methods. I'm vintage :D

>> # If running outside X try connecting to main display
>> try:
>>     display = os.environ['DISPLAY']
>> except KeyError:
>>     os.environ['DISPLAY'] = ':0.0'
> 
> This is weird. I haven't seen any program behaving that way. It's user
> responsibility to set DISPLAY correctly, and I wouldn't want any program
> to second-guess me on that matter.

Removed guessing main display when DISPLAY variable isn't set: it will
be user responsibility to correctly set it (e.g. if he wants to connect
to the display from tty1).

>>             wall.communicate(message)[0]
> 
> This looks a bit odd. Was the expression supposed to be assigned to
> something? If not, the "[0]" part is redundant.

Fixed. It was assigned to something in the past.

>>             pidf.close
> 
> This is no-op. I guess it should be: "pidf.close()".

Fixed.

> Catching all exceptions is almost always wrong (unless you re-raise them
> later), for two reasons:

Exception handling fixed.

>>             if e.errno != errno.EEXIST:
> 
> You didn't import the errno module.

Import added.

>>     gettext.install('gnome-session-shutdown', LOCALE_DIR, unicode=1)
> 
> AFAIK if you can omit the LOCALE_DIR part, Python will do the right
> thing. No need to hard-code path to locale directory.

Removed hard-coded locale directory.

>>         for fname in os.listdir('/proc'):
>>             if fname.isdigit():
>>                 if int(fname) >= 1000:
> 
> This looks very dubious to me. Why ">= 1000"?

Condition removed. I wrongly assumed a value of RESERVED_PIDS = 1000 in
kernel's PID allocator.

> Running pygettext on the source file results in:
> 
> $ pygettext gnome-session-shutdown
> *** gnome-session-shutdown:131: Seen unexpected token "+"
> *** gnome-session-shutdown:375: Seen unexpected token "+"
> *** gnome-session-shutdown:397: Seen unexpected token "+"
> *** gnome-session-shutdown:429: Seen unexpected token "+"

Fixed.

Thank you very much for your help.

Best

-- 
Jacopo Lorenzetti