Add clap for parsing command line arguments #52
Conversation
|
@polybuildr Can you suggest something to fix the CI error? |
|
Hello! Thanks a lot for the PR, and sorry for the delay in review. Just about the CI errors:
|
src/main.rs
Outdated
| let mut app = App::new("balloon") | ||
| .version("0.1.0") | ||
| .arg(Arg::with_name("file") | ||
| .help("run the file") |
There was a problem hiding this comment.
I'm a little confused, what is the difference between this and run? They have the same .help :)
There was a problem hiding this comment.
I put file to deal with balloon program.bl and run for balloon --run program.bl
They're the same because run is the default mode.
If we use something like --mode, then I can put run as the default value if the user doesn't supply one, but I can't think of another alternative for the current scenario.
What do you suggest?
There was a problem hiding this comment.
I think the easy way out for now is to simply leave a comment explaning why both are the same (just mention "default mode" or something). Eventually we'll stabilise the command line API and hopefully then, I'll have a sensible API that does not require duplication. :P
src/main.rs
Outdated
| }; | ||
| } | ||
| else { | ||
| run_file(m.value_of("file").unwrap(), AstWalkInterpreter::new()); |
There was a problem hiding this comment.
Avoid unwrap please, we can handle the error a little more gracefully with expect.
There was a problem hiding this comment.
Is there a possibility of a panic at this line?
There was a problem hiding this comment.
Perhaps not right now, but someone could unintentionally change the code later :)
There was a problem hiding this comment.
If I'm reading this correctly, and the unwrap is for the .value_of, then simply switching to the if-let pattern described elsewhere will remove the need for the unwrap, solving this problem.
src/main.rs
Outdated
| repl::run_repl(LLVMInterpreter::new()); | ||
| } | ||
| else if m.is_present("run") { | ||
| run_file(m.value_of("run").unwrap(), AstWalkInterpreter::new()) |
There was a problem hiding this comment.
I dislike this pattern of checking with is_present and then calling unwrap, it defeats the entire purpose of having an Option type in the language. We can write this as:
if let Some(f) = m.value_of("run") {
run_file(f, AstWalkInterpreter::new())
}
else if let Some(f) = m.is_present("check") {
typecheck_file(f).unwrap());
}
...There was a problem hiding this comment.
Right, I was kind of confused about the whole Option thing in the beginning.
I'll change this, thanks.
There was a problem hiding this comment.
Just a note, you can also use match if you feel that is more suited to the task. Sometimes if-let is nicer, sometimes match is. if-let is completely fine here, I'm just leaving options for you.
There was a problem hiding this comment.
Great work on this PR! :) Once again, thanks for the help.
Left a few comments inline, hopefully helping the discussion. Also, just checking, but you have built and tested this locally, right? I will do that once myself, but I'd prefer that both of us did it. :P
src/main.rs
Outdated
| let mut app = App::new("balloon") | ||
| .version("0.1.0") | ||
| .arg(Arg::with_name("file") | ||
| .help("run the file") |
There was a problem hiding this comment.
I think the easy way out for now is to simply leave a comment explaning why both are the same (just mention "default mode" or something). Eventually we'll stabilise the command line API and hopefully then, I'll have a sensible API that does not require duplication. :P
src/main.rs
Outdated
| repl::run_repl(LLVMInterpreter::new()); | ||
| } | ||
| else if m.is_present("run") { | ||
| run_file(m.value_of("run").unwrap(), AstWalkInterpreter::new()) |
There was a problem hiding this comment.
Just a note, you can also use match if you feel that is more suited to the task. Sometimes if-let is nicer, sometimes match is. if-let is completely fine here, I'm just leaving options for you.
src/main.rs
Outdated
| }; | ||
| } | ||
| else { | ||
| run_file(m.value_of("file").unwrap(), AstWalkInterpreter::new()); |
There was a problem hiding this comment.
If I'm reading this correctly, and the unwrap is for the .value_of, then simply switching to the if-let pattern described elsewhere will remove the need for the unwrap, solving this problem.
|
I ran it locally and it seems to work as expected. How do you want to deal with completion? |
|
I'll look at the code again and run it myself once. Will also try to resolve the CI errors. I think we should hold off |
|
Sorry, bad click! ... hold off on autocomplete for now, because the API isn't stable. Clap being able to generate it is pretty awesome! Since you've so kindly implemented the usage of Clap, we should be able to do this easily later. :) |
Fixes #41