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
Merged

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Apr 20, 2019

This PR refactors main.rs to use dedicated functions for different deno <subcommand>, as discussed in #2084

EDIT: This PR should be blocked by #2113

EDIT2: As suggested by @kevinkassimo in #2102 and changed --types and --prefetch to respective subcommands:

deno --types -> deno types
deno --prefetch -> deno prefetch

}).map_err(|(err, _worker)| print_err_and_exit(err))
});
tokio_util::run(main_future);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@ry PTAL at it. It seem that execute_mod_async does not evaluate and run file if prefetch is set to true, so simple letting worker finish the job seems to work fine. I'm not sure though

@bartlomieju bartlomieju force-pushed the refactor_cli_entry_point branch 2 times, most recently from c51aed8 to 691aea7 Compare April 20, 2019 13:53
@bartlomieju bartlomieju changed the title [WIP] Refactor CLI entry point Refactor CLI entry point Apr 20, 2019
cli/flags.rs Outdated Show resolved Hide resolved
cli/flags.rs Show resolved Hide resolved
cli/flags.rs Outdated Show resolved Hide resolved
cli/main.rs Outdated Show resolved Hide resolved
}
);
}
fn flags_from_vec(args: Vec<String>) -> DenoFlags {
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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().

Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively parse_flags may return DenoFlags, ArgMatches to handle both cases

cli/flags.rs Show resolved Hide resolved
cli/main.rs Outdated Show resolved Hide resolved
cli/main.rs Outdated Show resolved Hide resolved
@J2P
Copy link
Contributor

J2P commented Apr 21, 2019

The manual.md file also seems to need to be modified.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

very nice clean up - thanks !

@ry
Copy link
Member

ry commented Apr 21, 2019

PS we should add a "deno help" command - it feels like that would be expected now.

@bartlomieju
Copy link
Member Author

@ry no problem, I can add it in an hour or so. I understand you want to add deno help as an alias for deno --help, right?

@ry ry merged commit cd19da6 into denoland:master Apr 21, 2019
@bartlomieju bartlomieju deleted the refactor_cli_entry_point branch April 21, 2019 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants