-
Notifications
You must be signed in to change notification settings - Fork 14
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
test(commands): command input parsing #305
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
|
// consistent with the other platforms. | ||
#[allow(clippy::unnecessary_wraps)] | ||
#[cfg(windows)] | ||
fn split(s: &str) -> RusticResult<Vec<String>> { |
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.
with this, this is a change and not only a PR about tests;-)
I experimented shortly on winsplit. One drawback I found was that it seems to be impossible to have the reverse operation of split
- this makes a suitable Display
impl quite hard.
Also I just wanted to note that a OS-dependent parsing may cause irritation for users using both windows and non-windows. I don't use windows, but are there cases where shellwords::split
don't work on windows commands?
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.
Yeah, I'm playing around with it to get a feeling for the parsing. It's true, that we need to be attentive here.
just one thing: Did you see the tests in |
Thanks for the heads-up, no I didn't, I was actually wondering! |
No description provided.