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

Add deno eval command #2102

Merged
merged 9 commits into from Apr 13, 2019

Conversation

5 participants
@bartlomieju
Copy link
Contributor

bartlomieju commented Apr 12, 2019

This PR add deno eval command. Closes #2081

At the moment it's a proof of concept. Before merging this PR I'd like to land cleanup of CLI/flags as there's a lot of stutter in the code and it can be much cleaner.

TODO:

  • tests
  • better help for eval
@zekth

This comment has been minimized.

Copy link
Contributor

zekth commented Apr 12, 2019

Do a shortcut -e is revelant to add? like

deno -e console.log('foo')

@bartlomieju bartlomieju marked this pull request as ready for review Apr 12, 2019

bartlomieju added some commits Apr 12, 2019

Show resolved Hide resolved cli/flags.rs Outdated
Show resolved Hide resolved cli/main.rs Outdated
Show resolved Hide resolved cli/main.rs
@ry
Copy link
Collaborator

ry left a comment

Cool! Looks good : )

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Apr 13, 2019

As an aside should --types and --prefetch also be subcommands? (Perhaps prefetch could be "compile"...)

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Apr 13, 2019

@hayd yes I was thinking that too

@kevinkassimo

This comment has been minimized.

Copy link
Contributor

kevinkassimo commented Apr 13, 2019

Just for disambiguation, I believe something like deno run main.ts could also be introduced: for deno <subcommand> <script-name>, if <subcommand> does not match any existing subcommands, treat <subcommand> as <script-name>.

E.g.

  • deno run main.ts: Runs script of name main.ts
  • deno main.ts: Equivalent to deno run main.ts
  • deno eval "console.log('Hi');": evaluate code console.log('Hi');
  • deno run eval: Runs script of name ./eval
  • deno run run: Runs script of name ./run

(Since we have *.headers.json (previously *.mime) files, it is totally valid to have a file named eval without .ts extension but still treated as a TypeScript file due to content-type)

I also agree that we should promote prefetch and types as subcommands and no longer just flags. Actually, I'm not so sure if we could go even further (fully separating subcommands with flags, and no longer allow prefixing -- to subcommands):

  • subcommands: help, prefetch, version, types, info, run, fmt
  • flags: --allow-*, --log-debug, --reload, and others.

Personally believe this would make the functionalities much clearer. In this case, flags are more or less modifiers, which do not change the general behavior that subcommands dictate but decorate how to run and what to display.

E.g. The following seems very clear to me about what kind of output to expect:

  • deno run --allow-all main.ts
  • deno info main.ts
  • deno info --log-debug main.ts (we still want to show info, but we also want to show some Deno debug messages`
@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

bartlomieju commented Apr 13, 2019

Thanks for the review guys.

I do agree on having types and prefetch subcommands suggested by @hayd as well as version and run by @kevinkassimo. As for help... I'm not totally sure. -h/--help is pretty much muscle memory for everyone using CLIs at this moment. That said there's nothing preventing having both deno help and deno -h.

Adding all those subcommands would implicate refactor of main.rs to have a different entry points for all subcommands which I very much like and even started this refactor. But that will be in separate PR.

@ry

ry approved these changes Apr 13, 2019

Copy link
Collaborator

ry left a comment

LGTM - this will be great!

@ry ry merged commit 591b5e4 into denoland:master Apr 13, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Apr 13, 2019

@bartlomieju you could also have --help and --version as hidden args, so they work as expected but aren't in the help text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.