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

No prompts flag for non interactive environments. #1913

Merged
merged 8 commits into from Mar 13, 2019

Conversation

3 participants
@afinch7
Copy link
Contributor

commented Mar 12, 2019

Fixes #1580

@hayd

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

Does atty not already do this?

if !atty::is(atty::Stream::Stdin) || !atty::is(atty::Stream::Stderr) {

@afinch7

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

That only checks if stdio is a tty. Not really the same as the user simply wanting to disable all prompts regardless of tty.

@@ -149,6 +153,7 @@ pub fn set_flags(
opts.optflag("", "allow-env", "Allow environment access");
opts.optflag("", "allow-run", "Allow running subprocesses");
opts.optflag("A", "allow-all", "Allow all permissions");
opts.optflag("", "no-prompt", "Do not use prompts");

This comment has been minimized.

Copy link
@ry

ry Mar 12, 2019

Collaborator

--deny ?
#1580

This comment has been minimized.

Copy link
@afinch7

afinch7 Mar 12, 2019

Author Contributor

No prompt shouldn't be limited to permissions, so in that respect it's different than --deny. Either your there to answer for prompts or you aren't as a user.

This comment has been minimized.

Copy link
@hayd

hayd Mar 12, 2019

Contributor

What other prompts are there?

I don't understand the difference.

This comment has been minimized.

Copy link
@afinch7

afinch7 Mar 12, 2019

Author Contributor

I guess for the time being it only affects permissions, but If we add any other prompts in the future it should be used to allow disabling those as well.

This comment has been minimized.

Copy link
@ry

ry Mar 12, 2019

Collaborator

--deny would be confusing when combined with, say, --allow-read

> deno  foo.ts --allow-read --deny

What is intended is: "run deno with foo.ts as the main script, allow fs read access, but do not prompt for permission"
But people might interpret this as: "run deno with foo.ts as the main script, allow fs read access, deny all access (overriding previous?)"

This comment has been minimized.

Copy link
@ry

ry Mar 12, 2019

Collaborator

--no-prompt is kinda long.... Can we quickly just throw out a few other suggestions?

This comment has been minimized.

Copy link
@ry

ry Mar 12, 2019

Collaborator

No other suggestions it seems. I can't think of anything better...

This comment has been minimized.

Copy link
@hayd

hayd Mar 12, 2019

Contributor

what about --no (as analogous to --yes e.g. of unix tools)

}
r
} else {
Err(permission_denied())

This comment has been minimized.

Copy link
@ry

ry Mar 12, 2019

Collaborator

Seems like there are 4 very similar sections of code here. Is it possible to "DRY" this up?

This comment has been minimized.

Copy link
@afinch7

afinch7 Mar 12, 2019

Author Contributor

That should be much cleaner.

@ry

ry approved these changes Mar 12, 2019

Copy link
Collaborator

left a comment

This looks well implemented to me. I think --no-prompts is a fine name.

My only hesitation is that there aren't any tests... I realize it's a bit tricky to test things like this, but I wonder if you've looked at adapting tools/permission_prompt_test.py ? Is it possible to have some test of this flag using the technique there?

@hayd

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

https://github.com/denoland/deno/pull/1591/files has some relevant tests

Edit: I see you got in there already! 👍

@ry

ry approved these changes Mar 12, 2019

Copy link
Collaborator

left a comment

LGTM - thank you for the tests!

@ry
Copy link
Collaborator

left a comment

Err- it looks like the tests aren't being called. See line 196 of tools/permission_prompt_test.py

afinch7 added some commits Mar 13, 2019

@@ -98,6 +98,7 @@ impl DenoPermissions {
r
}

/// Try to present the user with a permission prompt permission_denied if no_prompts is enabled

This comment has been minimized.

Copy link
@ry

ry Mar 13, 2019

Collaborator

Sorry - can you wrap this at 80 cols?

Odd that rustfmt isn’t doing that...

Also this will give us a chance to sample from the appveyor failure distraction again

afinch7 added some commits Mar 13, 2019

fmt

@afinch7 afinch7 force-pushed the afinch7:no-prompt-flag branch from 5395a33 to 82dea1d Mar 13, 2019

@ry ry referenced this pull request Mar 13, 2019

Merged

v0.3.3 #1924

@ry

ry approved these changes Mar 13, 2019

Copy link
Collaborator

left a comment

LGTM - looks like its green now.

I think there are some spurious failures in the appveyor build - but it seems unrelated.

@ry ry merged commit 7e09221 into denoland:master Mar 13, 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
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.