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

Feat/add required flags #110

Closed

Conversation

HackAfro
Copy link

@HackAfro HackAfro commented Jan 12, 2019

Fixes #51.

This pull request attempts adding the feature to enable required flags.

Examples:

  1. Making a flag required:
const cli = meow({
	description: 'Custom description',
	help: `
		Usage
		  foo <input>
  `,
	flags: {
		foo: {
			alias: 'o',
			required: true
		}
	}
});

Output without required flag:

Missing required option
  --foo, -o

Run with --help to view help information.
  1. Creating provisional required flags. These flags are only required if another flag is present.
const cli = meow({
	description: 'Custom description',
	help: `
		Usage
		  foo <input>
  `,
	flags: {
		unicorn: {alias: 'u'},
		fence: {
			alias: 'f',
			required(args) {
				return args.unicorn;
			}
		},
	}
});

fixture --unicorn="horns"

The command above would make fence a required flag. Meaning fence is only required if unicorn is present.

@sindresorhus
Copy link
Owner

Please don't do unrelated changes. Revert the Prettier changes. Unrelated changes makes it very hard to review the actual changes.

@sindresorhus
Copy link
Owner

Missing required option

I think that should be option => flag

@sindresorhus
Copy link
Owner

From #51 (comment):

I also think we should call the option isRequired, not required.

@sindresorhus
Copy link
Owner

This is also unanswered:

Note, I'm not exactly sure what the function arguments should be. I'm open to suggestions there. I guess the input and flags. Anything else?

@@ -13,7 +13,17 @@ const cli = meow({
flags: {
unicorn: {alias: 'u'},
meow: {default: 'dog'},
camelCaseOption: {default: 'foo'}
camelCaseOption: {default: 'foo'},
fence: {
Copy link
Owner

Choose a reason for hiding this comment

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

It would be better to create a new fixture file for just testing the required flag stuff.

Copy link
Author

Choose a reason for hiding this comment

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

Okay. Will include this

t.is(stdout, 'Missing required option:\n --foo, -o\n\nRun with --help to view help information.');
});

test('provisional require', async t => {
Copy link
Owner

Choose a reason for hiding this comment

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

The title needs to be clearer. I'm not sure what exactly is being tested here.

Copy link
Author

Choose a reason for hiding this comment

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

The test is for optional required flags. Will update the description to make it clearer

'cat'
]);

t.is(i, 'Missing required option:\n --fence, -f\n\nRun with --help to view help information.');
Copy link
Owner

Choose a reason for hiding this comment

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

You also need a test for the positive side, when --fence is actually given.

]);

t.is(i, 'Missing required option:\n --fence, -f\n\nRun with --help to view help information.');
t.is(f, 'Missing required option:\n --foo, -o\n\nRun with --help to view help information.');
Copy link
Owner

Choose a reason for hiding this comment

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

It would be better to test only the important part of the message, so other parts are free to change in the future without having to update all the tests. You can use t.regex instead.

All we need to test is Missing required option:\n --foo, -o.

]);
t.is(
help,
indentString('\nCustom description\n\nUsage\n foo <input>\n\n', 2));
Copy link
Owner

Choose a reason for hiding this comment

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

Use t.regex here to simplify the assertion.

@HackAfro
Copy link
Author

This is also unanswered:

Note, I'm not exactly sure what the function arguments should be. I'm open to suggestions there. I guess the input and flags. Anything else?

I think the function arguments should be the flags or the args rather. Allowing the user to check the availability of any of the inputted flags

@sindresorhus
Copy link
Owner

@HackAfro Still interested in finishing this?

@HackAfro
Copy link
Author

Yes. Let me make these changes and update the PR

@sindresorhus
Copy link
Owner

@HackAfro Bump

@sindresorhus
Copy link
Owner

Closing for lack of response.

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.

Support required flags
2 participants