-
Notifications
You must be signed in to change notification settings - Fork 757
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
classlib: String:openOS escaping #5322
Conversation
In my experience |
IMO there are still two reasons to change it: macOS users would still gain better escaping that now, and SC codebase would be a bit more consistent |
Agreed that there is no reason to use systemCmd here -- but I have never seen openOS block the language until the window is closed on any platform (in multiple class sessions with students in Mac and Windows, and Linux on my own machine). The AFAICS the format of the Windows implementation is not substantially different from the others -- it appears to require an additional title argument but otherwise similar. So I guess it could be fixed simply by |
I don't know why, but on my system (running Arch Linux), This made me realize that if we go for I'm adding |
Hmmm... |
After looking at this again - I think that both the escaping fix and changing to |
Looking at it again too, I also don't see it necessary to add an I've edited and rebased the branch, we're ready to merge for me. |
Thanks @elgiano ! |
Not at all, just confused between code-styles :) thanks for catching that, I also added semicolons at the end |
Did you happen to test on Windows? I don' have a Windows machine handy right at this moment. |
Also, I've changed the title to reflect the current state of the PR. Maybe it wouldn't hurt to remove/cross out the parts about blocking from the first comment so that it's clear if someone looks at this in the future. |
Nope, I also don't have one
…On Wed, 2 Mar 2022, 22:39 Marcin Pączkowski, ***@***.***> wrote:
Did you happen to test on Windows? I don' have a Windows machine handy
right at this moment.
—
Reply to this email directly, view it on GitHub
<#5322 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACNMWRUHATWIJBKZIY4KDLU57NZNANCNFSM4WEN3YJA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sounds good, I'll test later today and will report back. |
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.
LGTM, thanks @elgiano !
Using an older build I just encountered an issue with |
Purpose and Motivation
Fixes #2297.
String:openOS calls systemCmd, resulting in:
This PR switches to SequenceableCollection:unixCmd, as suggested by @nhthn, fixing both problems at once.
I've changed implementations for Linux and macOS, but not for Windows, which I don't understand and can't test.
Types of changes
could anyone rely on openOS being blocking? I don't see it very likely, as openOS is not a file-chooser and doesn't return anything.
To-do list