-
Notifications
You must be signed in to change notification settings - Fork 15
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
OS Windows support (for the release branch) #11
Conversation
…ify fzf executable path, update version
@nk412 I reopened my pull request for the release branch. What do you think about it? |
pyfzf/pyfzf.py
Outdated
|
||
# Invoke fzf externally and write to output file | ||
self.sh['-c', f"cat {input_file.name} | fzf {fzf_options} > {output_file.name}"] & FG | ||
if os.name == "nt": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified. All platforms can be supported using the subprocess interface, so we don't need a branch just for Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good. I think the README can do with some updating as well, especially around the ability to provide a custom path for fzf.
Once this is merged to release
, I can tidy it up there!
Thanks again for this.
I made improvements to the prompt method, now it looks simpler and works the same on Windows and Linux (I ran short tests). Also fixed a bug with incorrect behavior in the absence of fzf Now only the standard os module is used, subprocess is not used at all. Also, the logic of working with temporary files has been changed to support the Windows platform |
@brookite 0.3.0 merged. Also updated the README. |
This is a duplicate of my pull request for the release branch. This commit fixes issue #8
What has been done:
-Removed plumbum dependency (now pyfzf uses only standard library modules)
-Added OS Windows support
-Ability to specify fzf executable path