5:08 p.m.
Please review the patch with fixes for ticket 536 - Catch the keyboard
interrupt during the execution of pkispawn and pkidestroy.
--Abhishek
10:15 a.m.
On 3/20/2013 5:08 PM, Abhishek Koneru wrote:
Please review the patch with fixes for ticket 536 - Catch the
keyboard
interrupt during the execution of pkispawn and pkidestroy.
Some comments:
1. Please add a short subject line in the comment like the other patches.
2. Any reason you use signal handler instead of try-except?
3. If we keep the signal handler, the signal.signal() invocation should
be moved into the main program so it's easier to read.
4. Does it print a single blank line between the interrupted line and
the 'canceled' message? See the example:
https://fedorahosted.org/pki/ticket/536
5. The US spelling is 'canceled' instead of 'cancelled':
http://grammarist.com/spelling/cancel/
I think in the future we'll provide a proper translation & spelling for
each locale, but to be consistent for now let's use US spelling as the
default.
--
Endi S. Dewata
3:04 p.m.
New subject: [PATCH] 40-2 Comments on patch 40 addressed
Please review the attached patch.
On Fri, 2013-03-22 at 10:15 -0500, Endi Sukma Dewata wrote:
On 3/20/2013 5:08 PM, Abhishek Koneru wrote:
> Please review the patch with fixes for ticket 536 - Catch the keyboard
> interrupt during the execution of pkispawn and pkidestroy.
Some comments:
1. Please add a short subject line in the comment like the other patches.
Comment modified to have a short subject line
2. Any reason you use signal handler instead of try-except?
3. If we keep the signal handler, the signal.signal() invocation should
be moved into the main program so it's easier to read.
Decided to go with signal handler rather than KeyboardInterrupt after
discussing with Endi. Moved the handler invocation to main program.
4. Does it print a single blank line between the interrupted line and
the 'canceled' message? See the example:
https://fedorahosted.org/pki/ticket/536
Message follows the example in ticket.
5. The US spelling is 'canceled' instead of 'cancelled':
http://grammarist.com/spelling/cancel/
I think in the future we'll provide a proper translation & spelling for
each locale, but to be consistent for now let's use US spelling as the
default.
--Abhishek
1:53 p.m.
New subject: [PATCH] 40-2 Comments on patch 40 addressed
On 3/25/2013 3:04 PM, Abhishek Koneru wrote:
Please review the attached patch.
A few more things: The signal handler in pkidestroy says 'Installation'
instead of 'Uninstallation'. There are some trailing whitespaces.
Other than that it's ACKed. Please fix before push.
--
Endi S. Dewata
2:36 p.m.
New subject: [PATCH] 40-2 Comments on patch 40 addressed
Fixed the comments, pushed to master.
--Abhishek
On Tue, 2013-03-26 at 13:53 -0500, Endi Sukma Dewata wrote:
On 3/25/2013 3:04 PM, Abhishek Koneru wrote:
> Please review the attached patch.
A few more things: The signal handler in pkidestroy says 'Installation'
instead of 'Uninstallation'. There are some trailing whitespaces.
Other than that it's ACKed. Please fix before push.
4288
days inactive
4294
days old
4 comments
2 participants
participants (2)
-
Abhishek Koneru -
Endi Sukma Dewata