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

feat: bring back deno <script> #2451

Merged
merged 8 commits into from Jun 5, 2019

Conversation

2 participants
@bartlomieju
Copy link
Contributor

commented Jun 4, 2019

Ref #2446.

This PR brings back ability to run scripts using deno <script> command.

bartlomieju added some commits Jun 4, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Current status:

$ deno // open REPL, no flags allowed
$ deno --foo // unrecognized arg, exit
$ deno --allow-net script.ts // run script.ts with net perm
$ deno run --allow-net script.ts // run script.ts with net perm
$ deno --allow-read run --allow-net script.ts // run script.ts with net perm

Please notice that "read" permission is ignored in last example, I'll try to get it working but it's tricky.

So such test case fails:

  #[test]
  fn test_flags_from_vec_25() {
    let (flags, subcommand, argv) =
      flags_from_vec(svec!["deno", "--allow-net", "run", "--allow-read", "script.ts"]);
    assert_eq!(
      flags,
      DenoFlags {
        allow_net: true,
        allow_read: true,
        ..DenoFlags::default()
      }
    );
    assert_eq!(subcommand, DenoSubcommand::Run);
    assert_eq!(argv, svec!["deno", "script.ts"]);
  }

CC @ry @MarkTiedemann @kitsonk @hayd

Show resolved Hide resolved cli/flags.rs Outdated
}
);
assert_eq!(subcommand, DenoSubcommand::Run);
assert_eq!(argv, svec!["deno", "script.ts"]);

This comment has been minimized.

Copy link
@ry

ry Jun 5, 2019

Collaborator

👍

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

Working for me

@bartlomieju bartlomieju changed the title feat: bring back deno <script> feat: bring back deno <script> [do not merge] Jun 5, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@ry please don't merge it yet, I need to correct logic for flag parsing after all

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

@bartlomieju consider trying speculative url parsing

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@ry sure. I need to update logic for parsing anyway, because this PR in current state will break mixed flag usages as well:

$ deno -r -D run --allow-net script.ts // -r -D would be ignored
@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@ry after some thought, I am not sure if we really need to try and parse URL?

For main module we use root_specifier_to_url which is a bit more relaxed than import URL parsing. If it's not a valid specifier or module is not found main.rs::run_script will error anyway. I don't see reason to add more logic to flag parsing to verify the URL. Thoughts?

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

root_specifier_to_url is more appropriate yes

bartlomieju added some commits Jun 5, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-cli_deno_run branch from e8f46a3 to c9f57b7 Jun 5, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Okay parsing logic is fixed, now both of the examples below will work properly:

$ deno --allow-read run --allow-net script.ts
$ deno -r -D run --allow-net script.ts

I also updated the manual with updated CLI usage

@bartlomieju bartlomieju changed the title feat: bring back deno <script> [do not merge] feat: bring back deno <script> Jun 5, 2019

Show resolved Hide resolved cli/flags.rs Outdated

bartlomieju added some commits Jun 5, 2019

@ry

ry approved these changes Jun 5, 2019

Copy link
Collaborator

left a comment

LGTM - nice to have this back but now properly integrated into the subcommand system.

@ry ry merged commit 6fa4d2e into denoland:master Jun 5, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@bartlomieju bartlomieju deleted the bartlomieju:feat-cli_deno_run branch Jun 5, 2019

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.