-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
permission prompt #1081
permission prompt #1081
Conversation
Nice cleanup! |
Looks like pty is Unix only. Perhaps we should skip these tests on Windows for now? |
@piscisaureus Please review regarding testing a TTY in windows. |
filename | ||
));; | ||
if r.is_ok() { | ||
self.allow_write = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this behavior is a little weird: You're asked if it's okay for deno to write foo.txt, you say yes, and deno can write anything for the rest of time.
IMO for now we should not use the filename/domain arg until we have finer permissions, either:
"🔐 Deno requests write access.
for the rest of execution."🔐 Deno requests write access to \"{}\"."
but ask every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine for a first pass. This feature is about ergonomics and clicking "yes" a bunch of times is annoying. Maybe we can change the wording to reflect the fact that access is granted for the rest of execution.
emoji doesn't work in windows - leaving it out. Bert tested windows manually - there's no easy way to test there. We're going to skip the test in Windows for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. The second patch is a bit weird -- does it change anything at all, it looks like it just moves some code around?
@piscisaureus re second patch - importing pty on win fails - so the imports have to be inside the conditional. |
- Add URLSearchParams (denoland#1049) - Implement clone for FetchResponse (denoland#1054) - Use content-type headers when importing from URLs. (denoland#1020) - Use checkJs option, JavaScript will be type checked and users can supply JSDoc type annotations that will be enforced by Deno (denoland#1068) - Add separate http/https cache dirs to DENO_DIR (denoland#971) - Support https in fetch. (denoland#1100) - Add chmod/chmodSync on unix (denoland#1088) - Remove broken features: --deps and trace() (denoland#1103) - Ergonomics: Prompt TTY for permission escalation (denoland#1081)
- Add URLSearchParams (#1049) - Implement clone for FetchResponse (#1054) - Use content-type headers when importing from URLs. (#1020) - Use checkJs option, JavaScript will be type checked and users can supply JSDoc type annotations that will be enforced by Deno (#1068) - Add separate http/https cache dirs to DENO_DIR (#971) - Support https in fetch. (#1100) - Add chmod/chmodSync on unix (#1088) - Remove broken features: --deps and trace() (#1103) - Ergonomics: Prompt TTY for permission escalation (#1081)
No description provided.