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

Switch to double quotes for escaping #114

Merged
merged 1 commit into from
Nov 3, 2024

Conversation

Wikiwix
Copy link
Contributor

@Wikiwix Wikiwix commented Nov 3, 2024

When the extension runs in Windows, for cli commands CMD is used. CMD does not treat single quotes as a special character which causes problems

When the extension runs in Windows, for cli commands CMD is used.
CMD does not treat single quotes as a special character which causes problems
@MichaelCurrin
Copy link
Owner

Oh thanks for the PR and details

@MichaelCurrin MichaelCurrin merged commit 0fd1754 into MichaelCurrin:master Nov 3, 2024
2 checks passed
@MichaelCurrin
Copy link
Owner

I've published the change, let me know if it works for you

@Wikiwix Wikiwix deleted the fix-escaping branch November 3, 2024 21:39
@Wikiwix
Copy link
Contributor Author

Wikiwix commented Nov 3, 2024

The new version works on Windows :)

Thanks for the quick merge!

Something that I thought of in the meantime:
IMHO, ideally the command should not be called via a shell at all (instead running the command with a real array of arguments). With that setup, no quoting should get in the way, regardless of the operating system.
Looking at the used exec function though, I do not see a variant with this format (I do see one for execFile and spawn)…

@MichaelCurrin MichaelCurrin mentioned this pull request Nov 4, 2024
@MichaelCurrin
Copy link
Owner

I tried execa package and that seems to work well for this specific case, but has other issues

#115

@MichaelCurrin
Copy link
Owner

This is the only flag in cli.ts that has quotes so I don't think it's worth changing how the app compiles just for that.

Plus, removing the quotes in string might work too for Windows + Unix. But if it's working now as is, I'll not tinker more with removing quotes.

@Wikiwix
Copy link
Contributor Author

Wikiwix commented Nov 11, 2024

@MichaelCurrin I thought about removing quotes altogether too, and decided against it, because = is a special character in sh too (not under all circumstances, but I wanted to be on the safe side)

And I do agree, that keeping it as is, until another issue manifests is the reasonable thing to do :)

I added a comment on the implementation in #115 regarding what I was aiming for with the call without the use of a shell :)

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