No prompts flag for non interactive environments. #1913
Conversation
|
Does atty not already do this? Line 152 in 77d7ad6 |
|
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"); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What other prompts are there?
I don't understand the difference.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
--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?)"
There was a problem hiding this comment.
--no-prompt is kinda long.... Can we quickly just throw out a few other suggestions?
There was a problem hiding this comment.
No other suggestions it seems. I can't think of anything better...
There was a problem hiding this comment.
what about --no (as analogous to --yes e.g. of unix tools)
src/permissions.rs
Outdated
| } | ||
| r | ||
| } else { | ||
| Err(permission_denied()) |
There was a problem hiding this comment.
Seems like there are 4 very similar sections of code here. Is it possible to "DRY" this up?
There was a problem hiding this comment.
That should be much cleaner.
There was a problem hiding this 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?
|
https://github.com/denoland/deno/pull/1591/files has some relevant tests Edit: I see you got in there already! 👍 |
src/permissions.rs
Outdated
| @@ -98,6 +98,7 @@ impl DenoPermissions { | |||
| r | |||
| } | |||
|
|
|||
| /// Try to present the user with a permission prompt permission_denied if no_prompts is enabled | |||
There was a problem hiding this comment.
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
Fixes #1580