Code review comment for lp://staging/~nfultz/terminator/terminator

Revision history for this message
Neal Fultz (nfultz) wrote :

OK, pushed a fix; it's a little uglier, though. Just to be explicit,
there are four cases, one of them is supposed to fail:

./terminator -e man htop

terminator: error: Additional unexpected arguments found: ['htop']

./terminator -x man htop

works

./x-terminal-emulator -e man htop

works

./x-terminal-emulator -x man htop

works

On Sun, Aug 04, 2013 at 04:10:33PM -0000, Stephen Boddy wrote:
> Review: Needs Fixing
>
> Hi Neal. First, thanks for the contribution. Unfortunately I won't apply this as is. My main issue is that you are changing the behaviour of -x too, by making it refuse arguments. The standard x terminal only has -e, so -x behaviour is not relevant to it. Gnome Terminal does not modify -x behaviour in the wrapper script available. So right now your patch adds more alternative behaviour. I'd be accepting of something that did what the Gnome Terminal wrapper script does (although obviously not in Perl ;-) Either a Python rewrite or integrated into the main terminator script.
>
> Last (minor) point, we don't typically use camel case in the coding of Terminator, so is_X_terminal_emulator rather than isXTerminalEmulator.
>
>
> --
> https://code.launchpad.net/~nfultz/terminator/terminator/+merge/163457
> You are the owner of lp:~nfultz/terminator/terminator.

« Back to merge proposal