Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix a couple of issue with company-read-env #1937

Closed
wants to merge 3 commits into from

Conversation

@CeleritasCelery
Copy link
Contributor

@CeleritasCelery CeleritasCelery commented Feb 15, 2019

This PR does two things:

  1. Makes company-read-env handle directory names correctly
  2. Enables environment variable completion in read-file-name-function

Based on feedback from #1932

counsel.el Outdated Show resolved Hide resolved
reading environment variables should be present in all read-file-name
functions, not just counsel-find-file
@CeleritasCelery
Copy link
Contributor Author

@CeleritasCelery CeleritasCelery commented Mar 5, 2019

@abo-abo I now have a CA from gnu. Since this change is over 15 lines.

@abo-abo abo-abo closed this in bedeb02 Mar 6, 2019
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Mar 6, 2019

Thanks. Very convenient: it's like a second set of bookmarks!

abo-abo added a commit that referenced this issue Mar 6, 2019
* ivy.el (ivy-make-magic-action): Add.

Re #1937
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Mar 6, 2019

I like the idea of binding find-file bookmark-like completion to accessible single keys, so I've added one more and bound it to `.

What I like about this one is that it simply re-uses an existing action, which brings the advantage of the action being exposed and discoverable on the action list.

Maybe we could do the same for ivy-magic-read-file-env: turn it into an action, and have the $ binding re-use that action.

@CeleritasCelery CeleritasCelery deleted the env-dir branch Mar 9, 2019
@CeleritasCelery
Copy link
Contributor Author

@CeleritasCelery CeleritasCelery commented Mar 9, 2019

I like how you implemented the bookmark key to make it more discoverable. However that will not work for ivy-magic-read-file-env because it can't be an action. Since actions exit the minibuffer, and ivy-magic-read-file-env is available in read-file-name (where we are interested in the return value) we can't exit when changing directories. We could implement it as an action in counsel-find-file, but that seems redundant.

I feel like the current implementation is sufficiently "discoverable" because it behaves very similar to shells. People are used to typing in $foo in a terminal. Many will just do that naturally and be pleasantly surprised when it "just works".

Also it is unfortunate that users have no access to bookmarks when using read-file-name. For example you can't rename a file to a bookmark (with rename-file), but you can rename it to an environment variable. However if you implemented bookmarks with a recursive minibuffer so it would work in read-file-name, it can no longer function as an action.

astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
reading environment variables should be present in all read-file-name
functions, not just counsel-find-file

Fixes abo-abo#1937
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants