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

avoid callCommand . showCommandForUser #27

Merged
merged 2 commits into from
Apr 1, 2022

Conversation

aryairani
Copy link
Contributor

@aryairani aryairani commented Feb 17, 2022

We found (unisonweb/unison#1870) that even though the System.Process.showCommandForUser docs say:

Given a program p and arguments args, showCommandForUser p args returns a string suitable for pasting into /bin/sh (on Unix systems) or CMD.EXE (on Windows).

that while this string may have been suitable for pasting into CMD.EXE on Windows, it wasn't suitable for passing to System.Process.callCommand on Windows, which was causing failures.

This PR passes the command arguments directly to callProcess instead of collapsing them into single string and then trying to parse them apart again within callCommand.

The PR also removes a redundant import from README.md, which was causing our build to fail (we don't allow warnings).

Thanks for this library!

@aryairani aryairani force-pushed the topic/avoid-callCommand branch from db54f22 to 2fd3485 Compare February 17, 2022 07:25
@aryairani aryairani changed the title avoid callCommand + showCommandForUser avoid callCommand . showCommandForUser Feb 17, 2022
@chshersh chshersh added the enhancement New feature or request label Apr 1, 2022
Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aryairani Thanks a lot for the fix and such detailed explanation 🤗

@chshersh chshersh merged commit 782d5f4 into kowainik:main Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants