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

Add yudai code #61

Closed
wants to merge 12 commits into from
Closed

Add yudai code #61

wants to merge 12 commits into from

Conversation

willghatch
Copy link
Member

This merges in a bunch of code from the fork of @yudai. Several contributors to zaw are using this code in their forks, and at least one pull request submitted to the main zsh-users repo requires some of these commits. This commit represents a merger of Yudai's code up to his latest 5 commits, which add features that aren't all working for me.

@willghatch willghatch closed this Jun 22, 2015
@willghatch
Copy link
Member Author

I don't know why I made a pull request. Instead I just pushed to master, since I can do that now, and this code is already in use by several people, so it's been reviewed by more than just me.

@willghatch willghatch deleted the add-yudai branch June 22, 2015 18:38
@yudai
Copy link
Contributor

yudai commented Jun 22, 2015

Thank you for merging! 👍

@yudai
Copy link
Contributor

yudai commented Jun 22, 2015

Btw, this commit 45a411c changes the default behavior and that can bring up some confusion for most users. If it's not required by other commits, it might be better to revert this one.

@willghatch
Copy link
Member Author

Actually, I saw that it changed the default behavior. It was actually in
the section that had merge conflicts (essentially formatting) with
something else I merged, so while I was cleaning that up I switched the
previous default back.

Thanks for your work. I only merged that first set of commits and not the
later ones because I had some issues with the inplace setting and some
other stuff you added. However, I like the functionality, and actually
added a pull request myself that does some of it (albeit a slightly
different way) before I found your code. I didn't merge it yet, in order
for the other new maintainer to be able to review it, so if you like the
way your patch works better you should make a pull request and we can
discuss it.

Anyway, you should definitely make pull requests with any future work you
do, and me or the other guy will get stuff merged in.

On Mon, Jun 22, 2015 at 12:59 PM, Iwasaki Yudai notifications@github.com
wrote:

Btw, this commit 45a411c
45a411c
changes the default behavior and that can bring up some confusion for most
users. If it's not required by other commits, it might be better to revert
this one.


Reply to this email directly or view it on GitHub
#61 (comment).

@yudai
Copy link
Contributor

yudai commented Jun 22, 2015

Got it.

As for the inplace search, I feel like my code is a little bit hacky as I was not so familiar with zsh/zaw and wrote it in a "anyway, it looks it's working" way. So if your PR works fine for the same purpose, it would be great to drop my commits and adopt your smarter way 👍

Here is a video for reference:
https://pbs.twimg.com/tweet_video/CA0ufoYVAAAMFbw.mp4

@willghatch
Copy link
Member Author

To clarify -- I added code to allow initial filter contents, primarily for the zaw-history source. I haven't done anything related to in place searching.

@yudai
Copy link
Contributor

yudai commented Jun 27, 2015

Thanks. I realized that your PR is for the initial filter pattern and updated/rebased my code with it.

My inplace search is just use the current shell line as the filter prompt. I'm not sure it makes sense for others. So let me know if you are interested in it. I'll create a PR then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants