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

Refactor CLI entry point #2157

Merged
merged 14 commits into from Apr 21, 2019
Next

refactor CLI entry point

  • Loading branch information...
bartlomieju committed Apr 20, 2019
commit bcd31fae9d943dc287c935a6d984e58ce90444d9
@@ -95,7 +95,7 @@ static ENV_VARIABLES_HELP: &str = "ENVIRONMENT VARIABLES:
DENO_DIR Set deno's base directory
NO_COLOR Set to disable color";

fn create_cli_app<'a, 'b>() -> App<'a, 'b> {
pub fn create_cli_app<'a, 'b>() -> App<'a, 'b> {
App::new("deno")
.bin_name("deno")
.global_settings(&[AppSettings::ColorNever])
@@ -198,46 +198,7 @@ fn create_cli_app<'a, 'b>() -> App<'a, 'b> {
}

#[cfg_attr(feature = "cargo-clippy", allow(stutter))]
pub fn set_flags(
args: Vec<String>,
) -> Result<(DenoFlags, Vec<String>), String> {
let mut rest_argv: Vec<String> = vec!["deno".to_string()];
let cli_app = create_cli_app();
let matches = cli_app.get_matches_from(args);

match matches.subcommand() {
("eval", Some(info_match)) => {
let code: &str = info_match.value_of("code").unwrap();
rest_argv.extend(vec![code.to_string()]);
}
("info", Some(info_match)) => {
let file: &str = info_match.value_of("file").unwrap();
rest_argv.extend(vec![file.to_string()]);
}
("fmt", Some(fmt_match)) => {
let files: Vec<String> = fmt_match
.values_of("files")
.unwrap()
.map(String::from)
.collect();
rest_argv.extend(files);
}
(script, Some(script_match)) => {
rest_argv.extend(vec![script.to_string()]);
// check if there are any extra arguments that should
// be passed to script
if script_match.is_present("") {
let script_args: Vec<String> = script_match
.values_of("")
.unwrap()
.map(String::from)
.collect();
rest_argv.extend(script_args);
}
}
_ => {}
}

pub fn set_flags(matches: ArgMatches) -> Result<DenoFlags, String> {
This conversation was marked as resolved by bartlomieju

This comment has been minimized.

Copy link
@ry

ry Apr 20, 2019

Collaborator

Regarding the return value - it doesn't seem like it ever errors? Can this be just

pub fn set_flags(matches: ArgMatches) -> DenoFlags

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Apr 20, 2019

Author Contributor

Yep, will do

This comment has been minimized.

Copy link
@ry

ry Apr 20, 2019

Collaborator

Can you also rename this function into something like "parse_flags" ? Add a triple slash comment that says it does not take any action, but only parses flags.

if matches.is_present("v8-options") {
// display v8 help and exit
// TODO(bartlomieju): this relies on `v8_set_flags` to swap `--v8-options` to help
@@ -256,138 +217,137 @@ pub fn set_flags(
}
This conversation was marked as resolved by bartlomieju

This comment has been minimized.

Copy link
@ry

ry Apr 20, 2019

Collaborator

v8_set_flags is called here, which makes it not a pure parsing function. Can that be moved elsewhere?

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Apr 20, 2019

Author Contributor

So based on this comment I'm gonna add v8_help and v8_flags fields to DenoFlags and call v8_set_flags in main.rs::main()

This comment has been minimized.

Copy link
@ry

ry Apr 20, 2019

Collaborator

Yes, sounds good.


let flags = DenoFlags::from(matches);
Ok((flags, rest_argv))
Ok(flags)
}

#[test]
fn test_set_flags_1() {
let (flags, rest) = set_flags(svec!["deno", "--version"]).unwrap();
assert_eq!(rest, svec!["deno"]);
assert_eq!(
flags,
DenoFlags {
version: true,
..DenoFlags::default()
}
);
}
#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_set_flags_2() {
let (flags, rest) =
set_flags(svec!["deno", "-r", "-D", "script.ts"]).unwrap();
assert_eq!(rest, svec!["deno", "script.ts"]);
assert_eq!(
flags,
DenoFlags {
log_debug: true,
reload: true,
..DenoFlags::default()
}
);
}
fn flags_from_vec(args: Vec<String>) -> DenoFlags {

This comment has been minimized.

Copy link
@ry

ry Apr 21, 2019

Collaborator

I feel like this function should be the high level public “parse_flags” while the other function which is currently called “parse_flags” should be either combined into this or called something else (and be internal to this module)

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Apr 21, 2019

Author Contributor

Yeah, I had similar feeling about it, but that means we no longer have matches var available in main(), that gives nice switch on matches.subcommand().

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Apr 21, 2019

Author Contributor

Alternatively parse_flags may return DenoFlags, ArgMatches to handle both cases

let cli_app = create_cli_app();
let matches = cli_app.get_matches_from(args);
set_flags(matches).unwrap()
}

#[test]
fn test_set_flags_3() {
let (flags, rest) =
set_flags(svec!["deno", "-r", "--allow-write", "script.ts"]).unwrap();
assert_eq!(rest, svec!["deno", "script.ts"]);
assert_eq!(
flags,
DenoFlags {
reload: true,
allow_write: true,
..DenoFlags::default()
}
);
}
#[test]
fn test_set_flags_1() {
let flags = flags_from_vec(svec!["deno", "--version"]);
assert_eq!(
flags,
DenoFlags {
version: true,
..DenoFlags::default()
}
);
}

#[test]
fn test_set_flags_4() {
let (flags, rest) =
set_flags(svec!["deno", "-Dr", "--allow-write", "script.ts"]).unwrap();
assert_eq!(rest, svec!["deno", "script.ts"]);
assert_eq!(
flags,
DenoFlags {
log_debug: true,
reload: true,
allow_write: true,
..DenoFlags::default()
}
);
}
#[test]
fn test_set_flags_2() {
let flags = flags_from_vec(svec!["deno", "-r", "-D", "script.ts"]);
assert_eq!(
flags,
DenoFlags {
log_debug: true,
reload: true,
..DenoFlags::default()
}
);
}

#[test]
fn test_set_flags_5() {
let (flags, rest) = set_flags(svec!["deno", "--types"]).unwrap();
assert_eq!(rest, svec!["deno"]);
assert_eq!(
flags,
DenoFlags {
types: true,
..DenoFlags::default()
}
)
}
#[test]
fn test_set_flags_3() {
let flags =
flags_from_vec(svec!["deno", "-r", "--allow-write", "script.ts"]);
assert_eq!(
flags,
DenoFlags {
reload: true,
allow_write: true,
..DenoFlags::default()
}
);
}

#[test]
fn test_set_flags_6() {
let (flags, rest) =
set_flags(svec!["deno", "--allow-net", "gist.ts", "--title", "X"]).unwrap();
assert_eq!(rest, svec!["deno", "gist.ts", "--title", "X"]);
assert_eq!(
flags,
DenoFlags {
allow_net: true,
..DenoFlags::default()
}
)
}
#[test]
fn test_set_flags_4() {
let flags =
flags_from_vec(svec!["deno", "-Dr", "--allow-write", "script.ts"]);
assert_eq!(
flags,
DenoFlags {
log_debug: true,
reload: true,
allow_write: true,
..DenoFlags::default()
}
);
}

#[test]
fn test_set_flags_7() {
let (flags, rest) =
set_flags(svec!["deno", "--allow-all", "gist.ts"]).unwrap();
assert_eq!(rest, svec!["deno", "gist.ts"]);
assert_eq!(
flags,
DenoFlags {
allow_net: true,
allow_env: true,
allow_run: true,
allow_read: true,
allow_write: true,
allow_high_precision: true,
..DenoFlags::default()
}
)
}
#[test]
fn test_set_flags_5() {
let flags = flags_from_vec(svec!["deno", "--types"]);
assert_eq!(
flags,
DenoFlags {
types: true,
..DenoFlags::default()
}
)
}

#[test]
fn test_set_flags_8() {
let (flags, rest) =
set_flags(svec!["deno", "--allow-read", "gist.ts"]).unwrap();
assert_eq!(rest, svec!["deno", "gist.ts"]);
assert_eq!(
flags,
DenoFlags {
allow_read: true,
..DenoFlags::default()
}
)
}
#[test]
fn test_set_flags_6() {
let flags =
flags_from_vec(svec!["deno", "--allow-net", "gist.ts", "--title", "X"]);
assert_eq!(
flags,
DenoFlags {
allow_net: true,
..DenoFlags::default()
}
)
}

#[test]
fn test_set_flags_9() {
let (flags, rest) =
set_flags(svec!["deno", "--allow-high-precision", "script.ts"]).unwrap();
assert_eq!(rest, svec!["deno", "script.ts"]);
assert_eq!(
flags,
DenoFlags {
allow_high_precision: true,
..DenoFlags::default()
}
)
#[test]
fn test_set_flags_7() {
let flags = flags_from_vec(svec!["deno", "--allow-all", "gist.ts"]);
assert_eq!(
flags,
DenoFlags {
allow_net: true,
allow_env: true,
allow_run: true,
allow_read: true,
allow_write: true,
allow_high_precision: true,
..DenoFlags::default()
}
)
}

#[test]
fn test_set_flags_8() {
let flags = flags_from_vec(svec!["deno", "--allow-read", "gist.ts"]);
assert_eq!(
flags,
DenoFlags {
allow_read: true,
..DenoFlags::default()
}
)
}

#[test]
fn test_set_flags_9() {
let flags =
flags_from_vec(svec!["deno", "--allow-high-precision", "script.ts"]);
assert_eq!(
flags,
DenoFlags {
allow_high_precision: true,
..DenoFlags::default()
}
)
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.