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

Allow emojis to be force enabled with --emoji. #3126

Merged
merged 1 commit into from
May 12, 2017

Conversation

elyobo
Copy link
Contributor

@elyobo elyobo commented Apr 12, 2017

The previous behaviour had a default of enabling emojis for Macs, and
allowed explicitly disabling them.

This PR keeps that default but allows --emoji to enable for other
platforms if the user wants to. --no-emoji still works as before, but
due to the way that commander processes "--no-" flags (forcing the
default to true and overwriting the default value given in the .option
definition) it was necessary to switch the flag definition from
--no-emoji to --emoji.

Summary

Allows users to enable emojis on non-darwin platforms.

Emojis are a nice feature but are explicitly disabled on non-darwin platforms. There appears to be some interest in having them on other plaforms (see #960) and this allows that by making --emoji work as you'd expect (--no-emoji and --emoji are already accepted, but only --no-emoji works).

Test plan

On linux

# Forcing emojis off
$ node ./lib/cli/index.js --no-emoji
yarn install v0.24.0-0
[1/4] Resolving packages...
success Already up-to-date.
Done in 0.65s.

# Default behaviour; off on non-darwin
$ node ./lib/cli/index.js 
yarn install v0.24.0-0
[1/4] Resolving packages...
success Already up-to-date.
Done in 0.68s.

# Forcing emojis on
$ node ./lib/cli/index.js --emoji
yarn install v0.24.0-0
[1/4] 🔍  Resolving packages...
success Already up-to-date.
✨  Done in 0.67s.

Note that there is a flow type error in this PR related to process.stdout.isTTY, however this PR didn't add that check; it seems like it's a flow error that it wasn't already failing (I'm not familiar with flow though, so happy to be corrected here).

Another tweak would be to shift the process.stdout.isTTY check into the default value for commander.emoji, which would allow users to force emoji output even if stdout isn't a TTY - I'm not sure if this is ever desirable, but it's not possible to override at the moment and the change is simple.

@elyobo elyobo mentioned this pull request Apr 12, 2017
@elyobo
Copy link
Contributor Author

elyobo commented Apr 13, 2017

Test failures appear to be unrelated to the changes in this PR (tests are failing in the master from which I branched this).

@elyobo elyobo force-pushed the emoji-flag branch 2 times, most recently from 56d450d to bef14fb Compare April 25, 2017 23:25
The previous behaviour had a default of enabling emojis for Macs, and
allowed explicitly disabling them.

This PR keeps that default but allows `--emoji` to enable for other
platforms if the user wants to.  `--no-emoji` still works as before, but
due to the way that commander processes "--no-" flags (forcing the
default to true and overwriting the default value given in the `.option`
definition) it was necessary to switch the flag definition from
`--no-emoji` to `--emoji`.
@elyobo
Copy link
Contributor Author

elyobo commented May 5, 2017

Worked around the flow problems with isTTY, although it still checks it (possibly an error that it's not a flow error?).

@bestander
Copy link
Member

I think this will make emoji off by default, we have them on by default on Mac.

@elyobo
Copy link
Contributor Author

elyobo commented May 12, 2017

@bestander, this PR keeps the default behaviour as is - enabled for Mac, disabled for the rest.

commander.option(
  '--emoji',
  'enable emoji in output',
  process.platform === 'darwin',
);

The third option to commander.option sets the default value for the --emoji flag to true if the platform is darwin.

@bestander
Copy link
Member

Sorry, did not notice :)

@bestander bestander merged commit 10b82bb into yarnpkg:master May 12, 2017
@bestander
Copy link
Member

Thanks for the PR.
Emojis FTW!

@elyobo
Copy link
Contributor Author

elyobo commented May 12, 2017

NP, thanks for the merge, now I will no longer have emoji envy :D 🎉

@elyobo elyobo deleted the emoji-flag branch May 12, 2017 20:06
BYK pushed a commit that referenced this pull request Jun 29, 2017
**Summary**

Fixes #3764. Follow up to #3126. Adds the missing `[bool]` type
descripter to `commander` definitions.

**Test plan**

Run `yarn` on macOS and enjoy your emojis responsibly.
bestander pushed a commit that referenced this pull request Jun 30, 2017
**Summary**

Fixes #3764. Follow up to #3126. Adds the missing `[bool]` type
descripter to `commander` definitions.

**Test plan**

Run `yarn` on macOS and enjoy your emojis responsibly.
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.

2 participants