Skip to content
This repository has been archived by the owner on Dec 29, 2021. It is now read-only.

Introduce a glorious new cli arg splitter #29

Closed
wants to merge 2 commits into from

Conversation

killercup
Copy link
Collaborator

@killercup killercup commented Mar 23, 2017

Allow &str and &[&str] as input for command

Fixes #25

Currently based on #24. Rebase on master before merging

Allow &str and &[&str] as input for `command`

Fixes #25
@killercup killercup force-pushed the feature/split-args branch from aed80a1 to a04a0e1 Compare March 23, 2017 16:58
Copy link
Collaborator

@colin-kiegel colin-kiegel left a comment

Choose a reason for hiding this comment

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

Sorry, I commented in German - didn't notice. ^^

The general approach is very good, but I see some problems with the parsing implementation. IMO we should define some invariants which should hold, i.e. stringify!("...").to_cmd()] should equal ["..."].to_cmd() for any string "...".

} else {
in_quote.push(c);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ich glaube es gibt Randfälle, in denen sich das noch unerwartet verhalten könnte. Ich spiele mal Advocatus Diaboli:

assert_eq!(
    r#"echo lorem                           ipsum"#.to_cmd(),
    vec!["echo", "lorem", "ipsum"]
);
assert_eq!(
    r#"echo "lorem' ipsum" dolor "sit' amet" '"#.to_cmd(),
    vec!["echo", "lorem' ipsum", "dolor", "sit' amet"]
);
assert_eq!(
    r#"echo "lorem\" ipsum" dolor "sit\" amet"'"#.to_cmd(),
    vec!["echo", "lorem\" ipsum", "dolor", "sit\" amet"]
);

Bei echo ist das unkritisch, bei anderen Befehlen könnte es schon problematisch sein, wenn Argument 3 plötzlich an Position 4 steht, etc. Vielleicht kann man sich hier auch irgendein fuzzy-testing überlegen? Das macht es natürlich alles sehr sehr kompliziert. Wichtig wäre aber, dass es sich vorhersehbar verhält.

M.E. wäre folgende Invariante sehr erstrebenswert:

  • Wenn "..." ein gültiger Rust String ist (... sind Platzhalter), dann sollte stringify!("...").to_cmd()] gleich ["..."].to_cmd() sein.

Ich kenne die stringify! Methode nicht im Detail, vermute aber, dass auf einen String angewendet, JSON::from_str(stringify!("...")) weitestgehend die Identität sein sollte. Auf dieser Annahme basiert ja die aktuelle Makro-Logik. Auch hier wäre vielleicht mal eine genauere Recherche oder Fuzzy Testing sinnvoll. :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

r#"echo "lorem' ipsum" dolor "sit' amet" '"#.to_cmd()

Great test case! I was so focused on getting other stuff that I think I actually went a bit overboard with this: We don't really have to deal with nested quotes at all – just recognize and ignore escaped quotes, right?

I've also just seen this crate, but I think it's more complex than we need here. I don't want to emulate shell syntax itself, just split some args :)


I think I can add quickcheck to test the invariant, but I'm not sure JSON::from_str(stringify!("...")) is that reliable. We'll see :)

@colin-kiegel
Copy link
Collaborator

PS: I would concentrate on supporting "..." first and revisit '...' later, if at all. I thing it would be ok to reject the latter in a controlled way.

@killercup
Copy link
Collaborator Author

Let's put this on hold for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants