lib/plugin: support plugin pinning #119

jedahan · 2017-02-10T21:08:43Z

This adds geometry_prompt_plugin_check(), which will be run to determine if we are in a context where it makes sense to display the plugin

A plugin can be pinned by appending a * to skip this check.

How can we could remove the quoting requirement for pins?

GEOMETRY_PROMPT_PLUGINS+=(rustup*)

instead of

GEOMETRY_PROMPT_PLUGINS+=('rustup*')

jedahan · 2017-02-10T21:13:47Z

Some alternatives to `*` at the end

plugin. looks like a typo to me

export GEOMETRY_PROMPT_PLUGINS=(exec_time git node rustup.)

.plugin look like a pin, and less like a typo

export GEOMETRY_PROMPT_PLUGINS=(exec_time git node .rustup)

plugin! looks alarming

export GEOMETRY_PROMPT_PLUGINS=(exec_time git node rustup!)

desyncr

I have no issues with quoting. Using '*' is cool to me.

desyncr · 2017-02-10T23:03:38Z

Watch out for API breaking changes. Either we should be backward compatible or start tagging/versioning.

My concern is with existing custom plugins that might break with a geometry update.

jedahan · 2017-02-11T15:44:45Z

One thing I saw zplug do, which I like, is to release a version that prints out deprecation messages, then wait a month, then remove support. We can do that here, though we might be able to get away with just ignoring _check() if it does not exist, which would be backwards compatible.

frm

Overall looks pretty good to me. Just some notes on the reads and a note on updating the node plugin.

Thanks for taking the time to add checks to the current plugins!

frm · 2017-02-12T14:25:39Z

README.md

@@ -75,6 +76,12 @@ These plugins will load and display on the right prompt. You can check the
 documentation and configuration for each specific plugin in the
 [plugins](/plugins) directory.

+You can pin* a plugin by appending `*` to the end of the name. Geometry will always render the plugin.


I would add a note here on pinning, since when reading this document from the start you don't really get what it is. If I was a new user, I would think "wait, so plugins don't always render? Why not?"

What do you think of just prepending something like "Some plugins only render when you are in a given directory or in the presence of a given file. You can have them always render by pinning the plugin. (...)"?

Good idea, prepended a sentence about pinning

frm · 2017-02-12T14:27:52Z

plugins/README.md

@@ -23,15 +23,15 @@ configuration files.


 ```sh
-GEOMETRY_PROMPT_PLUGINS=(virtualenv docker_machine exec_time git hg)
+GEOMETRY_PROMPT_PLUGINS=(virtualenv docker_machine exec_time git hg 'rustup*')
 ```

 *Note: if you're not sure where to put geometry configs, just add them to your `.zshrc`*


For the same reasons as before, if I missed the pinning section, I would be confused with the 'rustup*'. Maybe add a link saying "if you're not sure what's with the * you can read about it here" ? What's your take?

Added a note beneath the current note

frm · 2017-02-12T14:28:29Z

plugins/README.md

+  # Do nothing if we're not in a repository
+  [ -d $PWD/.git ] || return 1
+}
+```


👍 nice example, clear explanation

frm · 2017-02-12T14:29:39Z

plugins/rustup/README.md

+```sh
+GEOMETRY_PROMPT_PLUGINS+=('rustup*')
+```
+


Note about pinning here? Since it is not required the plugin with the *

removed the pinning

frm · 2017-02-12T14:30:44Z

plugins/node/plugin.zsh

-        GEOMETRY_NPM_VERSION="$(npm --version 2> /dev/null)" 
-        echo "$GEOMETRY_NODE_NPM_VERSION $GEOMETRY_NODE_VERSION ($GEOMETRY_NPM_VERSION)"
-    fi
+    GEOMETRY_NODE_VERSION="$(node -v 2> /dev/null)"


Just a note: don't forget to update this with the code recently merged from #108

Rebased against master, will force push to this PR soon

frm · 2017-02-12T14:35:59Z

I'm good with 'plugin*', don't have any strong feelings on that. None of the other suggestions do it better IMO.

I moved the version discussion to #121 since a lot has been said in different PRs and issues. Might as well have a dedicated issue for that 😄

This adds geometry_prompt_plugin_check(), which will be run to determine if we are in a context where it makes sense to display the plugin A plugin can be pinned by appending a * to skip this check How can we could remove the quoting requirement for pins? GEOMETRY_PROMPT_PLUGINS+=(rustup*) instead of GEOMETRY_PROMPT_PLUGINS+=('rustup*')

jedahan · 2017-02-12T18:00:11Z

I really don't like the extra ritual for globbing, this patch proposes +plugin for pinning

jedahan · 2017-02-12T18:00:39Z

I also made it backwards compatible in that, if you don't have a check function for a plugin, it just runs render. That way we don't need to version yet.

frm · 2017-02-13T09:58:29Z

Looking great! @desyncr any takes?

desyncr

Two small things. Should be good to go 👍

desyncr · 2017-02-13T12:21:56Z

README.md

@@ -75,6 +76,13 @@ These plugins will load and display on the right prompt. You can check the
 documentation and configuration for each specific plugin in the
 [plugins](/plugins) directory.

+Some plugins only render when you are in a given directory or in the presence of a given file.
+You can have those plugins always render by pinning a `+` before the name


"before the name" => "before the name:"

desyncr · 2017-02-13T12:24:01Z

plugins/README.md

 ```

 *Note: if you're not sure where to put geometry configs, just add them to your `.zshrc`*

+*Note: the `+` before rustup means the plugin is pinned, and will always render, regardless of context*


"pinned" should like to the "Pinning" section.

jedahan · 2017-02-13T16:21:50Z

So, should I squash the three commits, rewrite the commit message, and push? Not sure what's good protocol.

frm · 2017-02-13T16:48:03Z

Precisely that. Squash, merge, push. Or just change the big green button on GitHub to say "Squash and merge" and hit that.

jedahan mentioned this pull request Feb 10, 2017

Initial proposal for 'starred' plugins #117

Closed

jedahan requested review from desyncr and frm February 10, 2017 21:17

jedahan self-assigned this Feb 10, 2017

desyncr reviewed Feb 10, 2017

View reviewed changes

frm requested changes Feb 12, 2017

View reviewed changes

frm mentioned this pull request Feb 12, 2017

Version tagging #121

Closed

Jonathan Dahan added 2 commits February 12, 2017 12:58

Change pin to +plugin, incorporate PR requests

1c79397

jedahan force-pushed the plugin-pinning branch from b7d380e to 1c79397 Compare February 12, 2017 18:00

frm approved these changes Feb 13, 2017

View reviewed changes

desyncr reviewed Feb 13, 2017

View reviewed changes

lib/plugin: small cleanup from review comments

8842e59

desyncr approved these changes Feb 13, 2017

View reviewed changes

jedahan merged commit 02836c5 into master Feb 13, 2017

jedahan deleted the plugin-pinning branch February 13, 2017 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/plugin: support plugin pinning #119

lib/plugin: support plugin pinning #119

jedahan commented Feb 10, 2017

jedahan commented Feb 10, 2017 •

edited

Loading

desyncr left a comment

desyncr commented Feb 10, 2017

jedahan commented Feb 11, 2017

frm left a comment

frm Feb 12, 2017

jedahan Feb 12, 2017

frm Feb 12, 2017

jedahan Feb 12, 2017 •

edited

Loading

frm Feb 12, 2017

frm Feb 12, 2017

jedahan Feb 12, 2017

frm Feb 12, 2017

jedahan Feb 12, 2017

frm commented Feb 12, 2017

jedahan commented Feb 12, 2017

jedahan commented Feb 12, 2017

frm commented Feb 13, 2017

desyncr left a comment

desyncr Feb 13, 2017

desyncr Feb 13, 2017

jedahan commented Feb 13, 2017

frm commented Feb 13, 2017

lib/plugin: support plugin pinning #119

lib/plugin: support plugin pinning #119

Conversation

jedahan commented Feb 10, 2017

jedahan commented Feb 10, 2017 • edited Loading

Some alternatives to * at the end

desyncr left a comment

Choose a reason for hiding this comment

desyncr commented Feb 10, 2017

jedahan commented Feb 11, 2017

frm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedahan Feb 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frm commented Feb 12, 2017

jedahan commented Feb 12, 2017

jedahan commented Feb 12, 2017

frm commented Feb 13, 2017

desyncr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedahan commented Feb 13, 2017

frm commented Feb 13, 2017

jedahan commented Feb 10, 2017 •

edited

Loading

Some alternatives to `*` at the end

jedahan Feb 12, 2017 •

edited

Loading