Re: [RFC][PATCH] `newuser` prompt theme

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

> new file mode 100644
> +++ b/Functions/Prompts/prompt_newuser_setup

At the risk of bikeshedding, I don't think that's a good name.  A name
should describe what a thing *is*, not what its intended use is; and I'm
not particularly fond of the implication that new users should be wary
of trying _other_ themes.  Besides, what if someone else were to come
along and post an alternative theme aimed at new users?

> @@ -0,0 +1,197 @@
> +readonly -ga sysexits=(
> +    USAGE
⋮
> +    CONFIG

Two interrelated scripts on zsh.org hardcode these constants too.
I wonder if we should provide these constants in a standard autoloaded
function or preset variable.

> +prompt_newuser_precmd() {
> +  local exitstatus=$?

It's not documented that precmd hooks can rely on $? to be the exit code
of the last command.  That does work, though (because callhookfunc()
calls doshfunc(noreturnval=1)), and seems useful, and I guess we won't
want to change it, so shall we document it?

> +  emulate -L zsh
> +
> +  psvar[1]=
> +  case $exitstatus in
> +    <128-> )
> +      psvar[1]="SIG$signals[exitstatus-127] "
> +      ;|
> +    <64-78> )
> +      psvar[1]="EX_$sysexits[exitstatus-63] "

Nice, I might adopt this.  Or, come to think of it, I might teach
PRINT_EXIT_VALUE to do this ☺

> +      ;|
> +    <1-> )
> +      psvar[1]+="($exitstatus)"
> +  esac
> +
> +  if ! [[ -v vcs_info_msg_0_ ]]; then
> +    zstyle ':vcs_info:*' check-for-staged-changes yes

First, no other prompt theme sets styles, so I'm not sure prompt themes
should be doing that.

Second, even if a prompt were to set styles, doing so in a precmd and
behind what _looks_ like a "first run" condition but is actually a "we
just changed directory" condition isn't exactly best practice.

Also, there are literally zero comments in the code.  That makes it
rather unmaintainable by anyone other than you — and I wonder if I
shouldn't say "other than present-you", since future-you may not fare
any better than present-me at understanding the code without comments.

> +    vcs_info
> +    if [[ -n $vcs_info_msg_0_ ]]; then
> +      RPS1="$vcs_info_msg_0_"
> +    else
> +      RPS1="$( prompt_newuser_format start right )"
> +    fi
> +  fi
> +}
> +
> +prompt_newuser_line-init() {
> +  emulate -L zsh
> +
> +  case $CONTEXT in
> +    start )

Nitpick, but could we please use both parentheses in new code?  That
helps my $EDITOR's jump-to-matching-paren functionality happier.

> +prompt_newuser_setup() {
> +  prompt_opts=( cr percent sp )
> +
> +  zstyle -e ':vcs_info:*' formats '
> +    reply=( "%u%c$( prompt_newuser_format start branch repo )" )
> +  '
> +  zstyle -e ':vcs_info:*' actionformats '
> +    reply=( "%u%c$( prompt_newuser_format start action repo )" )
> +  '
> +  zstyle -e ':vcs_info:*' stagedstr '
> +    reply=( "$( prompt_newuser_format start staged )" )
> +  '
> +  zstyle -e ':vcs_info:*' unstagedstr '
> +    reply=( "$( prompt_newuser_format start unstaged )" )
> +  '

Your theme does _nothing_ with the 'unstaged' style other than
pass it through verbatim.  That appears to be NIH.  Why shouldn't the
theme just advise people to set the vcs_info directly?

> +prompt_newuser_setup "$@"

Sorry, but I'm not at all sure I support accepting this part of the
patch.

More below.

> +++ b/Functions/Prompts/promptinit
> @@ -178,8 +177,13 @@ Use prompt -h <theme> for help on specific themes.'
>  
>         # Reset some commonly altered bits to the default
>         local hook
> -       for hook in chpwd precmd preexec periodic zshaddhistory zshexit; do
> -         add-zsh-hook -D "${hook}" "prompt_*_${hook}"
> +       for hook in chpwd precmd preexec periodic zshaddhistory zshexit \
> +           zsh_directory_name; do
> +         add-zsh-hook -D "$hook" "prompt_*_$hook"
> +       done
> +       for hook in isearch-exit isearch-update line-pre-redraw line-init \
> +           line-finish history-line-set keymap-select; do
> +         add-zle-hook-widget -D "$hook" "prompt_*_$hook"

All these should be documented, like prompt_${foo}_preview is.

Recommend to name these prompt_${foo}_bar-{isearch-exit,isearch-update,…,keymap-select}
for some fixed value of «bar» to avoid namespace issues (i.e., name
collisions between existing prompts and future hooks).

The promptinit changes are independent of the new theme.  They should be
a separate patch and could conceivably be applied separately.