[PATCH] 40 Catch the Keyboard interrupt during the execution of pkispawn
Please review the patch with fixes for ticket 536 - Catch the keyboard interrupt during the execution of pkispawn and pkidestroy. --Abhishek
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
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:
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
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
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.
participants (2)
-
Abhishek Koneru -
Endi Sukma Dewata