Fasd module #211

Closed
wants to merge 5 commits into
from

4 participants

@clvv

It works under my testings. There're probably many things that can be done the zsh-way, for instance "$(which fasd)". Unfortunately I don't know much "zshisms."

For further improvements, I think completion can be user congifurable vai zstyle. But then the cache process would become more complex.

Also I noticed fasd default alias will overide some omz aliases. Maybe that should be put into README?

@sorin-ionescu sorin-ionescu commented on an outdated diff Jul 1, 2012
modules/fasd/init.zsh
@@ -0,0 +1,40 @@
+#
+# Command-line productivity booster, offers quick access to files and directories.
+#
+# Authors:
+# Sorin Ionescu <sorin.ionescu@gmail.com>
+# Wei Dai <x@wei23.net>
+#
+
+if (( ! $+commands[fasd] )); then
+ return 1
+fi
+
+cache_file="${0:h}/cache.zsh"
+if [[ "$(which fasd)" -nt "$cache_file" || ! -s "$cache_file" ]]; then
@sorin-ionescu
Owner
sorin-ionescu added a line comment Jul 1, 2012

$(which fasd) is not necessary. It checks for its presence on line 9.

Zsh has a bunch of useful associative arrays: $commands, $aliases, $functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sorin-ionescu

Aliases that shadow system commands or that overwrite other module commands should definitely be mentioned in the README. I strive for module independence: No module alias hell.

The only one I see that it overrides is d for dirs -v, which is probably fine since anyone who uses fasd probably won't bother with the directory queue.

@clvv

Then should I include omodload 'completion'? I forgot to mention fasd depends on the completion in REAME.

Maybe it's easier if you pull and then do the modifications.

If not, do you prefer I rebase my commits or leave them as is?

@sorin-ionescu
@clvv

I think it's ready now. With fasd module and completion on, startup time increased by only about 2~3 ms on my machine.

However, there might be further improvements that can be done:
1. Checking the modification date of .zshrc, refresh cache if .zshrc is newer than the cache file. (but is checking "$HOME/.zshrc" the right way?)
2. Make completion configurable through ':omz:module:fasd:ccomp' and ':omz:module:fasd:wcomp' styles. Along with 1, cache can be refreshed when user change his / her settings.

@sorin-ionescu

That's wrong. It should be if [[ ! -s "$cache_file" ]]; then and nothing more.

The purpose is that when there's a new version of fasd, init code will be re-cached (please give it one more glance). I can't gurantee the init code is bug free.

Ok.

@sorin-ionescu sorin-ionescu commented on the diff Jul 4, 2012
modules/fasd/init.zsh
+# Authors:
+# Sorin Ionescu <sorin.ionescu@gmail.com>
+# Wei Dai <x@wei23.net>
+#
+
+if (( ! $+commands[fasd] )); then
+ return 1
+fi
+
+cache_file="${0:h}/cache.zsh"
+if [[ "${commands[fasd]}" -nt "$cache_file" || ! -s "$cache_file" ]]; then
+ # Base init arguments
+ init_args='posix-alias zsh-hook'
+
+ # Load zsh-{ccomp,wcomp} when completion is loaded
+ if zstyle -t ':omz:module:completion' loaded; then
@sorin-ionescu
Owner
sorin-ionescu added a line comment Jul 4, 2012

This is going to be a problem. If you run init with completion available then turn off completion, fasd should fail. You should check the availability of completion in the fasd code using if (( $+functions[compadd] ));, alternatively, you can create a _fasd completion file and the init code can add the fasd completion directory to $fpath.

@clvv
clvv added a line comment Jul 4, 2012

I've noticed this. Another solution, which I mentioned in one of my comments in the pull request, is to re-cache when users changes their .zshrc. Are you talking about turning off completion after zsh starts up or turning completion off in .zshrc? Is there a way to turn off completion after it's on? As much as I like to keep fasd self-contained in one file, maybe providing extra files for zsh could be a better solution. I'll look into it.

@sorin-ionescu
Owner
sorin-ionescu added a line comment Jul 4, 2012

Yes, it's possible to turn off completion after it's on, but I doubt anybody will do it since it requires to unfunction a bunch of functions. It's unlikely that anybody will do it. That said, fasd should be bullet-proof for those who do not use a Zsh configuration framework. You should always check that the completion functions are available or make your completion autoloadable (best).

@clvv
clvv added a line comment Jul 4, 2012

You're right. It is better to make less assumptions.

if (( $+functions[compadd] ));

Did you mean check compdef? I see zsh has compadd even with an empty .zshrc. It also says in the zsh manual that compdef is defined with compsys. Also, it seems that using compctl is fine.

Anyway, I experienmented with it a bit (clvv/fasd@3ea0a19). Please tell me if there's any problems or room for improvements. I know separate files for zsh would be ideal. However, installation would be more complicated and I'm reluctant to take the step.

@sorin-ionescu
Owner
sorin-ionescu added a line comment Jul 4, 2012
@clvv
clvv added a line comment Jul 4, 2012

I tried, but it seems very hard to move this line below to compsys and preserve whatever it is doing with compctl.

compctl -U -K _fasd_zsh_cmd_complete -V fasd -x 'C[-1,-*e],s[-]n[1,e]' -c - 'c[-1,-A][-1,-D]' -f -- fasd fasd_cd

Maybe you can give me some insights?

@sorin-ionescu
Owner
sorin-ionescu added a line comment Jul 4, 2012
@clvv
clvv added a line comment Jul 4, 2012

Thanks. I'll do that when I get more time on my hand. Though if compctl does not get removed, I don't see any problem in using it. It seems more robust (no init, less code) and easier to understand than compsys (at least to me).

What's your opinion on pulling this module? I'll do some more testing on the fix and merge it into master. But maybe you want to wait until I tag a new version.

@sorin-ionescu
Owner
sorin-ionescu added a line comment Jul 4, 2012
@clvv
clvv added a line comment Jul 5, 2012

Why replace z? Why not let people choose?

@sorin-ionescu
Owner
sorin-ionescu added a line comment Jul 5, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sorin-ionescu

Integration has been delayed by an annoying grep-related bug.

@sorin-ionescu

I do not wish to make a unilateral decision to replace z with fasd because z, for me, is good enough.

To the people who want fasd in favour of z, sound off.

@PaBLoX-CL

I am using fasd right now and I like it more than z. I used it for some time but lacked some functionally I find essential (file matching). Also, by default fasd includes an alias to z so you can use it like you were using z.

@stephenmckinney

👍 on fasd. I've been using for months with no issues.

@sorin-ionescu

@PaBLoX-CL and @stephenmckinney fasd in Prezto differs a bit. You should check out the issue/211 branch. It's been simplified a bit. I'm contemplating simplifying it further by other removing the comma query tab completion in favour of key bindings or the key bindings in favour of the comma query tab completion.

I generally do not like adding more key bindings, but I find the comma query weird.

@PaBLoX-CL

Yeah, it's kind of weird =/, but I think as you as already pointed a couple of times it's better to use the module as "vanilla" as possible. Though, a note in the module readme about how to set the keybinding for easy reference imho it's a good idea.

@sorin-ionescu

@PaBLoX-CL fasd targets multiple shells. A lot of what it defines in aliases, for example, is not relevant to Zsh.

@stephenmckinney

I think @PaBLoX-CL was just saying let's kill the keybindings and just use the aliases. I agree, unless someone finds them really handy.

Personally I only use j to jump dirs and use completion module for file and dir completion

@sorin-ionescu

I actually killed the aliases besides a custom j precisely because they are not useful.

@PaBLoX-CL
@sorin-ionescu

@PaBLoX-CL They are not useful in Zsh but are useful in BASH and other shells because they lack Zsh's completion capabilities. My j is an interactive z. I chose the interactive one because when there are multiple results, fasd's algorithm can select the wrong one. The interactive version gives the option to the user to choose when there are multiple results.

@PaBLoX-CL
@sorin-ionescu

Actually, fuzzy matching in fasd is not that powerful and it also has a length limit.

j allows you to select both files and directories, and when it doesn't know which to choose, like between prezto/ and zprezto/, it will give you the option before blindingly switching to the wrong directory.

As for file and directory completion for other problems, there are two options, weird comma stuff and key bindings.

Prezto is meant to be a KISS project, not include nor enable everything under the sun. Knowledgeable people can do so on their own in their own forks.

I'm tired of fixing stuff due to incompatibilities between modules. I have better things to do with my time. Oh My Zsh is a mess precisely because everything under the sun has been included and enabled without though for consequences.

In the case of fasd, only my custom alias, j, will be enabled and either the weird comma stuff or the key bindings, but not both.

@PaBLoX-CL
@sorin-ionescu
@PaBLoX-CL
@sorin-ionescu
@PaBLoX-CL
@sorin-ionescu

You can see how it's being loaded right now in this issue's branch.

@ColinHebert ColinHebert commented on the diff Sep 16, 2012
modules/fasd/README.md
+comma-separated query. Such formated command-line arguments will be tab
+completed via fasd.
+
+Authors
+-------
+
+*The authors of this module should be contacted via the [issue tracker][5].*
+
+ - [Sorin Ionescu](https://github.com/sorin-ionescu)
+ - [Wei Dai](https://github.com/clvv)
+
+[1]: https://github.com/clvv/fasd
+[2]: https://github.com/joelthelion/autojump
+[3]: https://github.com/rupa/z
+[4]: https://github.com/rupa/v
+[5]: https://github.com/sorin-ionescu/oh-my-zsh/issues
@ColinHebert
ColinHebert added a line comment Sep 16, 2012

This URL will need to be updated

@sorin-ionescu
Owner
sorin-ionescu added a line comment Sep 16, 2012

I rewrote it. See the branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@PaBLoX-CL

Fine by me :)

@sorin-ionescu sorin-ionescu added a commit that referenced this pull request Sep 30, 2012
@sorin-ionescu [#129, #211] Remove z b967c50
@sorin-ionescu sorin-ionescu added a commit that closed this pull request Sep 30, 2012
@sorin-ionescu [Fix #129, Fix #211] Add fasd 24bb99c
@lucy lucy added a commit to lucy/prezto that referenced this pull request Oct 1, 2012
@sorin-ionescu [#129, #211] Remove z 0aeb5a0
@lucy lucy added a commit to lucy/prezto that referenced this pull request Oct 1, 2012
@sorin-ionescu [Fix #129, Fix #211] Add fasd 68c1988
@pbrisbin pbrisbin added a commit to pbrisbin/prezto that referenced this pull request Oct 1, 2012
@pbrisbin pbrisbin Merge remote-tracking branch 'upstream/master'
* upstream/master: (113 commits)
  Fix SSH-Agent casing in README
  [Fix #300] Disable SSH-Agent protocol emulation by default
  Update external completions
  [Fix #129, Fix #211] Add fasd
  [#129, #211] Remove z
  Load dependencies after requirements check
  Add a missing space to peepcode
  Simplify mkdcd completion
  Add completion for Git submodule moving and removing
  Silence git-branch-current in aliases
  Make sure that the current directory is a Git repository
  Return inside of the if statement
  Simplify git-config calls
  Fix a #compdef bug introduced in 7dd7859
  Add missing backslashes
  Define variable expl as local
  Load dependencies in Git init
  [Fix #303] Remove extra '/' in sorin theme
  [Fix #301] Unset $MATCH after use
  Ensure $key_info is populated before use
  ...
ffca226
@Fl4t Fl4t added a commit to Fl4t/prezto that referenced this pull request Oct 6, 2012
@sorin-ionescu [#129, #211] Remove z c8e1b37
@Fl4t Fl4t added a commit to Fl4t/prezto that referenced this pull request Oct 6, 2012
@sorin-ionescu [Fix #129, Fix #211] Add fasd e7dfce1
@taybin taybin added a commit to taybin/prezto that referenced this pull request Oct 8, 2012
@taybin taybin Merge remote-tracking branch 'upstream/master'
* upstream/master: (56 commits)
  Replace table captions with headings
  Replace z with fasd in modules README
  [Fix #244] Add Git special action styles
  Simplify git-log zstyle documentation
  Fix the name of the theme setup function in READMEs
  [Fix #96] Add documentation for git
  Add zstyles for git-log formats
  Rename gk prefixed aliases to gC
  Rename git-info zstyle patterns
  Rename python-info zstyle patterns
  Rename ruby-info zstyle patterns
  Rename editor-info zstyle patterns
  Rename the Git status ignore submodules zstyle pattern
  Rename aliases gSu to gSI and gSU to gSu
  Rename the alias gsc to gsX
  Remove the gig alias
  Fix SSH-Agent casing in README
  [Fix #300] Disable SSH-Agent protocol emulation by default
  Update external completions
  [Fix #129, Fix #211] Add fasd
  ...
4cc4cea
@lildude lildude pushed a commit to lildude/prezto that referenced this pull request Jan 12, 2014
@sorin-ionescu [#129, #211] Remove z e56989b
@lildude lildude pushed a commit to lildude/prezto that referenced this pull request Jan 12, 2014
@sorin-ionescu [Fix #129, Fix #211] Add fasd 0545ad4
@lackac lackac added a commit to lackac/prezto that referenced this pull request Jan 19, 2014
@sorin-ionescu [#129, #211] Remove z 3f0dc97
@lackac lackac added a commit to lackac/prezto that referenced this pull request Jan 19, 2014
@sorin-ionescu [Fix #129, Fix #211] Add fasd 22e3a96
@sorin-ionescu sorin-ionescu added a commit that referenced this pull request Oct 10, 2014
@sorin-ionescu Revert "[Fix #129, Fix #211] Add fasd"
This reverts commit 24bb99c.
dc06819
@sorin-ionescu sorin-ionescu added a commit that referenced this pull request Oct 10, 2014
@sorin-ionescu Revert "[#129, #211] Remove z"
This reverts commit b967c50.
2d1881c
@suhsteve suhsteve pushed a commit that referenced this pull request Apr 19, 2016
@sorin-ionescu [#129, #211] Remove z 361e2c6
@suhsteve suhsteve pushed a commit that referenced this pull request Apr 19, 2016
@sorin-ionescu [Fix #129, Fix #211] Add fasd 736d312
@suhsteve suhsteve pushed a commit that referenced this pull request Apr 19, 2016
@sorin-ionescu Revert "[Fix #129, Fix #211] Add fasd"
This reverts commit 736d312.
a2ad2cc
@suhsteve suhsteve pushed a commit that referenced this pull request Apr 19, 2016
@sorin-ionescu Revert "[#129, #211] Remove z"
This reverts commit 361e2c6.
51e2f23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment