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

Propose strategy for dealing with color flags #46

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

wswoodruff
Copy link
Member

Not sure how I feel about what I came up with here:

  • Looking up the command definitions and looking for [*] to determine if it's a meta flag or not. In my brain I'm saying meta flags are flags that can be applied to any command, and I'm removing them from the equation for the rest of the logic so no code changes are needed in specific command paths besides referencing argsSansMetaFlags instead of extraArgs.
    • I didn't want to have 2 places where these special flags are defined so that's why I decided to lookup by the description. Lemme know what you think!

After approach is decided, I can write some tests to bring it back up to 100% coverage.

@coveralls
Copy link

coveralls commented Jul 27, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 7d8080b on wswoodruff:color-and-no-color-flags into dae9481 on hapipal:master.

@devinivy
Copy link
Member

devinivy commented Aug 2, 2020

Hey, thanks for this! I appreciate you moving the ball forward on this long-standing issue.

Here's the basic acceptance criteria:

  • Providing --no-color or --color as an argument should always be allowed.
  • The run command should still receive these arguments.
  • All other commands should move ahead as though those arguments have already been processed.

I think you nailed all these just by adding --color and --no-color to the bossy config. I believe the logic to exclude those flags from extraArgs is already taken care of after that. So you may be able to cut this concept of meta flags and a handful of lines of code. I also wouldn't worry about handling the case that both flags are specified at the same time— I'm fine saying that it's the supports-color module's job to deal with that. Even providing an error message will happen in color or not, so there's really no way you can win, ha! Curious what you think of all this. Thanks again for moving it forward :)

@wswoodruff
Copy link
Member Author

So any bossy flags will be stripped out if u grab args from args._ — but here we're grabbing options.argv for run commands — https://github.com/hapipal/hpal/blob/master/lib/index.js#L23

Should we grab args._ instead? Looks like in the run command we're piecing through the args, should we change that part?

@devinivy
Copy link
Member

devinivy commented Aug 7, 2020

The way it currently works is intentional: run commands should receive all arguments, and all other commands should only receive arguments that weren't processed by e.g. bossy.

@wswoodruff
Copy link
Member Author

Left off attempting to write tests for the --color and --no-color flags by trying to see how we can prove color is or isn't there. So I was looking at how chalk does it and that's as far as I got https://github.com/chalk/chalk/blob/main/test/chalk.js

@devinivy
Copy link
Member

devinivy commented Jan 29, 2021

@wswoodruff one way to do it could be to use StripAnsi(value) === value. If it is true the value with and without ansi is the same, so it must not have contained any ansi :) I am pretty sure the strip-ansi package is already used in the test suite.

@devinivy
Copy link
Member

By the way, if you add some tests around that I think you'll want to add it to the bin section of the tests, since I believe that is where supports-color is actually used: https://github.com/hapipal/hpal/blob/master/test/index.js#L1626

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.

3 participants