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

OS Windows support (for the release branch) #11

Merged
merged 2 commits into from
Jan 12, 2022
Merged

OS Windows support (for the release branch) #11

merged 2 commits into from
Jan 12, 2022

Conversation

brookite
Copy link
Contributor

@brookite brookite commented Jan 7, 2022

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

@brookite
Copy link
Contributor Author

brookite commented Jan 7, 2022

@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":
Copy link
Owner

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.

Copy link
Owner

@nk412 nk412 left a 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.

@brookite brookite requested a review from nk412 January 10, 2022 16:53
@brookite
Copy link
Contributor Author

brookite commented Jan 10, 2022

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

@nk412 nk412 merged commit 49438e3 into nk412:release Jan 12, 2022
@nk412 nk412 mentioned this pull request Feb 8, 2022
@nk412
Copy link
Owner

nk412 commented Feb 8, 2022

@brookite 0.3.0 merged. Also updated the README.
Thank you for this!

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