mklement0 / ttab

macOS and Linux CLI for opening a new terminal tab/window, optionally with a command to execute and/or display settings
285 stars 15 forks source link

Added options -c -v -h -i #72

Closed HofiOne closed 6 months ago

HofiOne commented 6 months ago

Resolves: #70 Resolves: #18

mklement0 commented 6 months ago

This looks very promising, thank you.

Some high-level questions / comments first:

HofiOne commented 6 months ago
* What is the value of the `-c` option? why wouldn't you just execute a command directly, or, if you were targeting a different terminal app with `-a` (from Terminal to iTerm, say), why would you want to execute something in the _current_ tab in the other app?

-c (similary to i) is useful if you invoke ttab from an external tool, like Apple Shortcuts, it can be useful for targeting the current terminal tab with output of a given external tool)

  * (As an aside: `-c` is currently broken in Terminal.app)

i'll take a look at this

* Rather than _documenting_ incompatible or ignored options, I prefer them to be prevented programmatically:

  * If `-g` / `-G` cannot be meaningfully combined with `-h` or `-v`, a syntax error should be reported.
  * Also, specifying _both_ `-h` and `-v` should be prevented.

not exactly agree on this (there are multiple examples of ignoring colliding options on first win bases), but as you wish

* In the synopsis part of the embedded man page, the description of iTerm-only options (`-h`, `-v`) should have an "iTerm only:" prefix.

* In the remainder of the embedded man page, please restore the two trailing spaces at the end of all lines, which are meant to ensure word wrapping for proper rendering in an 80-column terminal.

* There are typos: `Schortcuts` -> `Shortcuts`, `un-priviliged` -> `un-privileged`, or perhaps  better: `non-privileged`; also, I prefer not to use the word "the" in "via the environment variable TTAB_CMD_DELAY"

will take a look at these as well

HofiOne commented 6 months ago
also, I prefer not to use the word "the" in "via the environment variable TTAB_CMD_DELAY"

Sorry, i could not find any related changes by me around the word TTAB_CMD_DELAY Could you please add a comment on this in the review part

HofiOne commented 6 months ago

I hope I addressed now all of your concerns, please check them, also please use the GitHub review tool in case of any further comments as it is much easier to handle, and follow them one by one over there. Thank you!

HofiOne commented 6 months ago

There's quite a few changes I'm requesting, but most of them are minor. I do wonder if we need the -n option, however.

Hope I did not forget anything, please feel free to add further comments if needed.

HofiOne commented 6 months ago

There's a minor typo (tterm instead of ttab), but otherwise this looks great. I'll deal with the typo, as I'm planning on adding another check anyway.

Ahhh, shame on me, will you fix it, or should I send a new fix PR?

mklement0 commented 6 months ago

I'll fix it myself, thanks, and thanks again for your contribution.

I'll try to release a new version soon.

HofiOne commented 6 months ago

Oh, one more issue, sorry :( Probably you noticed too.

in line 398 of ttab

elif (( gnomeTerminal && inCurrent ))

must be

elif (( gnomeTerminal && inCurrent )); then

mklement0 commented 6 months ago

No worries; already fixed. v0.9.0 has now been published.

HofiOne commented 6 months ago

perfect, thank you!

HofiOne commented 6 months ago

btw, you might want to close this one as well https://github.com/mklement0/ttab/issues/18 cheers!