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: reorganize global flags #3389

Merged
merged 7 commits into from Nov 26, 2019

Conversation

@ry
Copy link
Collaborator

ry commented Nov 20, 2019

  • Remove ability to specify run arguments like --allow-net after the script argument. It's too hacky to make work with clap.
  • Prevent clap from auto-detecting terminal width and attempting to wrap text.
  • Remove --v8-options, instead use --v8-flags=--help
  • Give more descriptive names to unit tests in flags.rs
  • Assume argv and subcommand into DenoFlags struct so the output of flags module is only DenoFlags rather than the tuple (subcommand, flags, argv).
  • Improve CLI help text
  • Make deno run specific args like --allow-net only show up in 'deno
    help run' instead of as global flags in deno help.
  • Removes deno version to simplify our implementation and be closer to clap defaults. deno -V now only shows Deno's version and not V8's nor TypeScript. Deno.versions can be used to see that information.
@ry ry force-pushed the ry:flags2 branch from 6f05ac7 to 8f97985 Nov 23, 2019
@ry ry mentioned this pull request Nov 23, 2019
@ry ry force-pushed the ry:flags2 branch 3 times, most recently from 1502846 to 3b7d93d Nov 23, 2019
- Remove ability to specify run arguments like `--allow-net` after the
  script argument. It's too hacky to make work with clap.
- Remove `--v8-options`, instead use `--v8-flags=--help`
- Give more descriptive names to unit tests in flags.rs
- Assume argv and subcommand into DenoFlags struct so the output of
  flags module is only DenoFlags rather than the tuple (subcommand, flags,
  argv).
- Improve CLI help text
- Make `deno run` specific args like `--allow-net` only show up in 'deno
  help run' instead of as global flags in `deno help`.
- Removes `deno version` to simplify our implementation and be closer to
  clap defaults. `deno -V` now only shows Deno's version and not V8's nor
  TypeScript. `Deno.versions` can be used to see that information.
- Prevent clap from auto-detecting terminal width and attempting to wrap
  text.
@ry ry force-pushed the ry:flags2 branch from 127dcc5 to 1d70b93 Nov 23, 2019
Copy link
Contributor

nayeemrmn left a comment

--config, --no-fetch, --lock and --lock-write should apply to deno fetch as well. --lock-write shouldn't apply to deno test.

@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Nov 23, 2019

@nayeemrmn Why would --config apply to fetch? deno fetch --no-fetch $URL doesn't sound right. I agree about --lock and --lock-write.

@nayeemrmn

This comment has been minimized.

Copy link
Contributor

nayeemrmn commented Nov 23, 2019

@ry

Why would --config apply to fetch?

Doesn't it affect compilation?

deno fetch --no-fetch $URL doesn't sound right.

#3383 (comment). I agree that the naming is inconsistent. That was actually what prompted #3384.

@ry ry force-pushed the ry:flags2 branch from d35b4f8 to f958891 Nov 23, 2019
@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Nov 23, 2019

Doesn't it affect compilation?

Yes, ok. I agree and I've made the change.

Regarding deno fetch --no-fetch ... I don't understand your reasoning still.

ry added 2 commits Nov 23, 2019
fix
@nayeemrmn

This comment has been minimized.

Copy link
Contributor

nayeemrmn commented Nov 23, 2019

It's hard to reason about since I'm not sure what the use case of --no-fetch is in the first place...

Say I want to disable remote module downloading but I still want to use deno fetch to compile ahead of time. If deno fetch is forced to download everything, --no-fetch for deno run becomes useless since it's all already cached. I don't see why these two features should be at odds.

@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Nov 23, 2019

@nayeemrmn Ah ok. I understand now. But maybe we should call it something else like --recompile-only ? In any case let's do that in a follow up PR since it should have tests. This one's already getting too big.

@nayeemrmn

This comment has been minimized.

Copy link
Contributor

nayeemrmn commented Nov 23, 2019

@ry None of this will matter after #3384 😅 I suggest just applying it as-is for now and certainly not introducing another flag.

fix
cli/flags.rs Show resolved Hide resolved
cli/flags.rs Show resolved Hide resolved
cli/flags.rs Show resolved Hide resolved
cli/flags.rs Show resolved Hide resolved
@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Nov 24, 2019

If we were to rename fetch to compile I think the flag would make a lot more sense i.e. deno compile --no-fetch.

I know @ry you wanted deno compile to mean "compile into a single binary including deno itself" but perhaps that would be better as deno binary or something like that...

cli/flags.rs Show resolved Hide resolved
Copy link
Collaborator

piscisaureus left a comment

I am down with the changes to how flags are interpreted and I see no issues with the code.
That said it's a very long patch. Are there areas in particular that you think need some extra eyeballs?
Also, has this been addressed? #3389 (comment)

Tentatively approved.

ry added 2 commits Nov 26, 2019
@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Nov 26, 2019

That said it's a very long patch. Are there areas in particular that you think need some extra eyeballs?

Nope - I think this is good to go

Also, has this been addressed? #3389 (comment)

Now it has.

@ry ry merged commit c016684 into denoland:master Nov 26, 2019
6 checks passed
6 checks passed
test macOS-latest
Details
test windows-2019
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
@ry ry deleted the ry:flags2 branch Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.