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
--
To UNSUBSCRIBE, email to debian-mentors-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/4fcedfad.9010...@cyan.xubiq.com
