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

Add emojis to permission prompts #1684

Merged
merged 2 commits into from Feb 7, 2019
Merged

Conversation

dsseng
Copy link
Contributor

@dsseng dsseng commented Feb 5, 2019

Reason: #1651. I think it looks much better now

@dsseng
Copy link
Contributor Author

dsseng commented Feb 5, 2019

I'll add emojis to other output strings too, but not too many.

src/permissions.rs Outdated Show resolved Hide resolved
src/permissions.rs Outdated Show resolved Hide resolved
@dsseng
Copy link
Contributor Author

dsseng commented Feb 5, 2019

@daynin fixed! Thanks for a good advice.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks -

Please use ⚠️... and no need to use a different one for each permission

It would be nice to make the text bold too... I think we could use
https://github.com/ogham/rust-ansi-term/blob/master/Cargo.toml
Or maybe it's easier just to write our own color module by porting this:
https://github.com/denoland/deno_std/blob/master/colors/mod.ts

@dsseng
Copy link
Contributor Author

dsseng commented Feb 5, 2019

@ry I think each emoji should say what is required, not just ⚠️

@ry
Copy link
Member

ry commented Feb 5, 2019

I disagree. The purpose of the emoji is just to distinguish an important message from other logging output - we should be consistent and minimal in applying these. This is not going to be one of these projects that splatters the screen with undiscerning graphics.

@dsseng
Copy link
Contributor Author

dsseng commented Feb 5, 2019

@ry done! But after adding the ansi_term crate python tools/build.py stopped working, but cargo build didn't.

src/permissions.rs Outdated Show resolved Hide resolved
src/permissions.rs Outdated Show resolved Hide resolved
src/permissions.rs Outdated Show resolved Hide resolved
@ry ry requested a review from piscisaureus February 5, 2019 18:10
@ry
Copy link
Member

ry commented Feb 5, 2019

@piscisaureus Please review for windows

src/permissions.rs Outdated Show resolved Hide resolved
src/permissions.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@piscisaureus
Copy link
Member

windows output visually looks like:

D:\deno>target\debug\deno.exe tools\format.ts
Compiling D:/deno/tools/format.ts
Compiling D:/deno/js/deps/https/deno.land/x/std/fs/path.ts
Compiling D:/deno/js/deps/https/deno.land/x/std/fs/path/mod.ts
Compiling D:/deno/js/deps/https/deno.land/x/std/fs/path/constants.ts
Compiling D:/deno/tools/util.ts
clang_format
�[1m�  Deno requests access to run a subprocess. Grant? [yN] �[0m

@dsseng
Copy link
Contributor Author

dsseng commented Feb 6, 2019

@piscisaureus are you in PowerShell or cmd?

@piscisaureus
Copy link
Member

@sh7dm cmd. But I don't think that matters.

@piscisaureus
Copy link
Member

PS D:\deno> .\target\debug\deno .\tools\format.ts
clang_format
�[1m�  Deno requests access to run a subprocess. Grant? [yN] �[0m

@ry
Copy link
Member

ry commented Feb 6, 2019

revert the last commit to make it work on windows....

Use #[cfg(windows)] above the line.

@ry
Copy link
Member

ry commented Feb 7, 2019

@piscisaureus please try again

@ry ry mentioned this pull request Feb 7, 2019
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - nice work!

@ry ry merged commit 5c50d28 into denoland:master Feb 7, 2019
@dsseng dsseng deleted the permission-emoji branch February 8, 2019 06:13
ry added a commit to ry/deno that referenced this pull request Feb 9, 2019
- Add deps to --info output (denoland#1720)
- Add --allow-read (denoland#1689)
- Add deno.isTTY() (denoland#1622)
- Add emojis to permission prompts (denoland#1684)
- Add basic WebAssembly support (denoland#1677)
- Add `NO_COLOR` support https://no-color.org/ (denoland#1716)
- Add color exceptions (denoland#1698)
- Fix: do not load cache files when recompile flag is set (denoland#1695)
- Upgrade V8 to 7.4.98 (denoland#1640)
ry added a commit that referenced this pull request Feb 9, 2019
- Add deps to --info output (#1720)
- Add --allow-read (#1689)
- Add deno.isTTY() (#1622)
- Add emojis to permission prompts (#1684)
- Add basic WebAssembly support (#1677)
- Add `NO_COLOR` support https://no-color.org/ (#1716)
- Add color exceptions (#1698)
- Fix: do not load cache files when recompile flag is set (#1695)
- Upgrade V8 to 7.4.98 (#1640)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants