From de76990950fa8ec0f74f901e7fdd76c74bdba472 Mon Sep 17 00:00:00 2001 From: Robby Russell Date: Sat, 5 Sep 2026 14:48:54 -0700 Subject: [PATCH] fix(cli): address review feedback on `omz generate plugin` - Turn %placeholders% into private markers when a template is read and strip the marker from every value, so text inserted from user input (e.g. -d '%name%') is never rescanned as a placeholder. - When --enable is used for a name already in $plugins (a custom override of an enabled built-in), report it and reload instead of failing in _omz::plugin::enable. - Make the completion note accurate whether or not a _ file was generated. - Test the write-phase cleanup with a template set missing a later template, and test that placeholder-looking values come out literally. Co-Authored-By: Claude Fable 5.1 --- lib/cli.zsh | 42 ++++++++++++------- lib/tests/generate-plugin.test.zsh | 25 +++++++++++ .../plugin/completion-note.zsh-template | 1 - 3 files changed, 53 insertions(+), 15 deletions(-) diff --git a/lib/cli.zsh b/lib/cli.zsh index c195cd9e7..afeff0d9e 100644 --- a/lib/cli.zsh +++ b/lib/cli.zsh @@ -429,7 +429,13 @@ function _omz::generate::plugin { # Last thing we do: in an interactive shell this restarts zsh if (( enable )); then print - _omz::plugin::enable "$name" + if (( ${plugins[(Ie)$name]} )); then + # Already enabled (e.g. a custom override of a built-in): just reload + _omz::log info "'$name' is already in your plugins list." + [[ ! -o interactive ]] || _omz::reload + else + _omz::plugin::enable "$name" + fi fi } @@ -516,31 +522,39 @@ function _omz::generate::plugin::ask { REPLY="${${REPLY##[[:space:]]#}%%[[:space:]]#}" } -# Print a template file, or explain which one is missing +# Print a template file with its %placeholders% turned into private markers, +# or explain which one is missing function _omz::generate::plugin::template { if [[ ! -f "$templates/$1" ]]; then _omz::log error "missing template '${templates/#$HOME/\~}/$1'." "omz::generate::plugin" return 1 fi - print -r -- "$(<"$templates/$1")" + + local content="$(<"$templates/$1")" token m=$'\x1f' + for token in completion cache name command description compgen; do + content="${content//\%${token}\%/${m}${token}${m}}" + done + print -r -- "$content" } -# Fill in the %placeholders% of a template and write it to a file. Values come +# Fill in the placeholders of a template and write it to a file. Values come # from the caller: $name, $cmd, $description, $compgen, $block and $cache. -# Substitution is done with parameter expansion rather than sed so that the -# values are always taken literally. +# +# Placeholders are matched as the markers that ::template produced, and the +# marker character is stripped from every value, so a value that happens to +# look like a placeholder (e.g. -d '%name%') is written out literally. function _omz::generate::plugin::render { setopt localoptions extendedglob - local content + local content m=$'\x1f' content="$(_omz::generate::plugin::template "$1")" || return 1 - # Blocks go first: they contain placeholders of their own - content="${content//'%completion%'/$block}" - content="${content//'%cache%'/$cache}" - content="${content//'%name%'/$name}" - content="${content//'%command%'/$cmd}" - content="${content//'%description%'/$description}" - content="${content//'%compgen%'/$compgen}" + # Blocks go first: they are templates too and carry markers of their own + content="${content//${m}completion${m}/$block}" + content="${content//${m}cache${m}/$cache}" + content="${content//${m}name${m}/${name//$m/}}" + content="${content//${m}command${m}/${cmd//$m/}}" + content="${content//${m}description${m}/${description//$m/}}" + content="${content//${m}compgen${m}/${compgen//$m/}}" # Drop blank lines left behind by an empty placeholder at the end content="${content%%$'\n'##}" diff --git a/lib/tests/generate-plugin.test.zsh b/lib/tests/generate-plugin.test.zsh index 375d2e745..fced5923b 100644 --- a/lib/tests/generate-plugin.test.zsh +++ b/lib/tests/generate-plugin.test.zsh @@ -75,6 +75,12 @@ else pass "foo has no unfilled placeholders" fi +## Values that look like placeholders are written literally + +omz generate plugin lit -d '%compgen% and %name% stay' -c lit --yes >/dev/null 2>&1 +assert_contains "$ZSH_CUSTOM/plugins/lit/README.md" '%compgen% and %name% stay' +assert_contains "$ZSH_CUSTOM/plugins/lit/lit.plugin.zsh" '%compgen% and %name% stay' + ## A plugin with no command if omz generate plugin bar --yes >/dev/null 2>&1; then @@ -151,6 +157,25 @@ else fi [[ -e "$ZSH_CUSTOM/plugins/quux" ]] && fail "quux was created by a failed run" +## A failure after the first file is written removes everything it created + +broken="$(mktemp -d)" +mkdir -p "$broken/templates/generators/plugin" "$broken/plugins" +cp "$ZSH/templates/generators/plugin/plugin.zsh-template" \ + "$ZSH/templates/generators/plugin/completion-note.zsh-template" "$broken/templates/generators/plugin/" +# README.md-template is missing, so the plugin file is written and then the README fails +if ( ZSH="$broken"; omz generate plugin partial -c partial --yes >/dev/null 2>&1 ); then + fail "succeeded with a missing README template" +else + pass "fails when a later template is missing" +fi +if [[ -e "$ZSH_CUSTOM/plugins/partial" ]]; then + fail "partial plugin directory was left behind: $(ls "$ZSH_CUSTOM/plugins/partial")" +else + pass "cleans up the partial plugin directory" +fi +command rm -rf "$broken" + ## The prompt helper _omz::generate::plugin::ask "Q" "dflt" 2>/dev/null < <(print '') diff --git a/templates/generators/plugin/completion-note.zsh-template b/templates/generators/plugin/completion-note.zsh-template index ec4a8af7d..e9b4b0b10 100644 --- a/templates/generators/plugin/completion-note.zsh-template +++ b/templates/generators/plugin/completion-note.zsh-template @@ -3,5 +3,4 @@ # # Oh My Zsh adds this directory to $fpath, so a file named `_%command%` next to # this one is picked up automatically as the completion for %command%. -# There is nothing to do here. #