Add `isRequired` flag option #141

sbencoding · 2020-03-24T21:29:01Z

This PR aims to add the ability to specify an isRequired option for flags, based on code and feedback from #110 and #51.

What I did

Implement the isRequired option. Can be a boolean or a function.
Update readme and type definition with the isRequired option.
Add tests and new fixture files for the new feature

Questions/suggestions

I think it's enough to pass the flags and the input variables to the isRequired function. That should give the consuming application enough context to decide the return value, at least from the package's point of view IMO.

In the original issue it was suggested that this should be entirely handled by the consuming application. I didn't know if I should, but it's possible to add an additional flag that changes the behavior of isRequired, so that instead of exiting and printing the missing options, it would include the missing flags in the result returned by the API.

I didn't add any additional tests to index.test-d.ts because I wasn't sure what to test for. isRequired either closes the app if some value is missing or leaves the value untouched if it's specified, so I don't know if the feature needs any type related testing.

Let me know what you think and if I should change anything.

Fixes #51

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

#51: Support required flags

IssueHunt has been backed by the following sponsors. Become a sponsor

index.js

sindresorhus · 2020-04-27T10:50:59Z

readme.md

@@ -97,6 +97,7 @@ The key is the flag name and the value is an object with any of:
 - `type`: Type of value. (Possible values: `string` `boolean` `number`)
 - `alias`: Usually used to define a short flag alias.
 - `default`: Default value when the flag is not specified.
+- `isRequired`: Boolean or Function that specified is this flag is required.


This needs to describe the parameters the function receives.

sindresorhus · 2020-04-27T10:51:12Z

readme.md

@@ -106,6 +107,9 @@ flags: {
 		type: 'string',
 		alias: 'u',
 		default: 'rainbow'
+		isRequired: (flags, input) => {
+			if (flags.otherFlag) return true;


sindresorhus · 2020-04-27T10:52:05Z

index.js

+	if (missing.length > 0) {
+		console.log(`Missing required option${missing.length > 1 ? 's' : ''}`);
+		for (const flag of missing) {
+			console.log(`\t--${flag.key}${flag.alias ? `, -${flag.alias}` : ''}`);


Shouldn't this be console.error?

sindresorhus · 2020-04-27T10:52:38Z

index.js

+
+	// Print error message for missing flags
+	if (missing.length > 0) {
+		console.log(`Missing required option${missing.length > 1 ? 's' : ''}`);


I think we should be consistent with the word "flag" instead of "option".

sindresorhus · 2020-04-27T10:52:51Z

index.js

@@ -118,6 +128,30 @@ const meow = (helpText, options) => {
 		}
 	}

+	// Get a list of missing flags, that are required
+	const missing = [];


"missing" what? Needs a more descriptive name.

sindresorhus · 2020-04-27T10:53:40Z

fixture-required.js

@@ -0,0 +1,26 @@
+#!/usr/bin/env node


There's a lot of fixtures now. Can you move them into a test/fixtures directory?

sindresorhus · 2020-04-27T10:54:49Z

test.js

@@ -80,6 +80,72 @@ test('spawn cli and test input flag', async t => {
 	t.is(stdout, 'bar');
 });

+test('spawn cli and test required flag with missing args', async t => {


Can you move these new tests into a new test file? And also move all tests into a test directory?

… descriptions

sbencoding · 2020-04-28T21:11:27Z

I've implemented the requested changes and merged with the latest master as well, let me know if there's anything else to change.

sindresorhus · 2020-05-05T15:29:56Z

index.d.ts

@@ -2,11 +2,13 @@ import {PackageJson} from 'type-fest';

 declare namespace meow {
 	type FlagType = 'string' | 'boolean' | 'number';
+	type IsRequiredPredicate = (flags: AnyFlags, input: string[]) => boolean;


Suggested change

type IsRequiredPredicate = (flags: AnyFlags, input: string[]) => boolean;

type IsRequiredPredicate = (flags: AnyFlags, input: readonly string[]) => boolean;

sindresorhus · 2020-05-05T15:32:12Z

readme.md

+- `isRequired`: Boolean or Function that specifies if this flag is required.
+	Two arguments are passed to the function.
+	The first arguments is the **flags** object, it contains the flags converted to camelCase excluding aliases.
+	The second arugment is the **input** string array, it contains the non-flag arguments.


sindresorhus · 2020-05-05T15:32:17Z

readme.md

+	Two arguments are passed to the function.
+	The first arguments is the **flags** object, it contains the flags converted to camelCase excluding aliases.
+	The second arugment is the **input** string array, it contains the non-flag arguments.
+	The function should return a Boolean, true if the flag is requried, otherwise false.


readme.md

sindresorhus · 2020-05-05T15:34:35Z

index.d.ts

+			Two arguments are passed to the function.
+			The first arguments is the flags object, it contains the flags converted to camelCase excluding aliases.
+			The second arugment is the input string array, it contains the non-flag arguments.
+			The function should return a Boolean, true if the flag is requried, otherwise false.


The function docs should be at https://github.com/sindresorhus/meow/pull/141/files#diff-b52768974e6bc0faccb7d4b75b162c99R5 and you don't have to mention the types. It's already shown by the actual types. Mention what it does and what problem it solves.

Yeah, I was just trying to make sure that the index.d.ts and the readme.md file reflect each other as much as possible, that's why I left the type information there, because although typescript has a better way of documenting type information, in the readme file this is the only way.
Anyways I left the documentation with the type information in the readme file and updated the typescript file with a better documentation

sindresorhus · 2020-05-05T15:35:47Z

test/test-is-required-flag.js

@@ -0,0 +1,72 @@
+import test from 'ava';


The filename can just be is-required-flag.js

sindresorhus · 2020-05-05T15:37:06Z

index.js

+
+const getMissingRequiredFlags = (flags, receivedFlags, input) => {
+	const missingRequiredFlags = [];
+	if (typeof flags !== 'undefined') {


Use an early-return here to reduce the nesting.

sbencoding · 2020-05-06T16:57:11Z

I've implemented the requested changes and merged with the current upstream master branch that contains the isMultiple flag.
Because of this merge I've added a few more test to make sure isRequired plays nicely with it.

sindresorhus · 2020-05-07T05:28:58Z

Looks good. Thanks :)

sbencoding added 4 commits March 24, 2020 21:30

Add isRequired option for flags

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

abd6b12

Add ability to specify isRequired as a function

3d2f6c8

Add type definition for isRequired

9b0c22a

Sync readme with typedef comments and examples

45fb69a

ulken mentioned this pull request Apr 23, 2020

Add isMultiple option for flags #143

Merged

sindresorhus requested changes Apr 27, 2020

View reviewed changes

sindresorhus changed the title ~~Implement isRequired feature as described in #51 and #110~~ Add isRequired flag option Apr 27, 2020

sbencoding added 9 commits April 28, 2020 21:04

Merge feature branch with upstream/master

aebcf85

Use console.error, better variable naming

6bde170

Update documentation

c095990

Use stderr, because missing flags are printed with console.error

1d9fa7d

Split out isRequired logic to separate methods

1f6423a

Organize tests and fixtures, update affected paths

a5c24a2

Split out isRequired flag related tests to separate file, better test…

bf0d816

… descriptions

Specify fixture path better

8b03f9f

Add isRequired (as function) return value to documentation

854e294

sindresorhus requested changes May 5, 2020

View reviewed changes

sbencoding added 8 commits May 6, 2020 16:28

Merge branch 'master' into feature-flag-is-required

e25cfdc

Fix broken state after merge

c54437d

Improve documentation

80cc740

Simplify code, check isRequired callback return value type

019dd32

Add test for isRequired callback return value validation

940fb96

Add handling of isMultiple flags to the isRequired logic

3c1c710

Add tests for isRequired and isMultiple options set at the same time

22f0a77

Fix estest path

e11a0ad

sindresorhus added 3 commits May 7, 2020 13:20

Update index.d.ts

e5dac8b

Update readme.md

1e04bdc

Update fixture-required-function.js

59de8b8

sindresorhus added 2 commits May 7, 2020 13:25

Update fixture-required-multiple.js

3dee237

Update fixture-required.js

4b7a7ac

sindresorhus merged commit 1eede6a into sindresorhus:master May 7, 2020

sbencoding deleted the feature-flag-is-required branch May 7, 2020 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Add `isRequired` flag option #141

Add `isRequired` flag option #141

sbencoding commented Mar 24, 2020 •

edited by sindresorhus

Loading

sindresorhus Apr 27, 2020

sindresorhus Apr 27, 2020

sindresorhus Apr 27, 2020

sindresorhus Apr 27, 2020

sindresorhus Apr 27, 2020

sindresorhus Apr 27, 2020

sindresorhus Apr 27, 2020

sbencoding commented Apr 28, 2020

sindresorhus May 5, 2020

sindresorhus May 5, 2020

sindresorhus May 5, 2020

sindresorhus May 5, 2020

sbencoding May 6, 2020

sindresorhus May 5, 2020

sindresorhus May 5, 2020

sbencoding commented May 6, 2020

sindresorhus commented May 7, 2020 •

edited

Loading

	type IsRequiredPredicate = (flags: AnyFlags, input: string[]) => boolean;
	type IsRequiredPredicate = (flags: AnyFlags, input: readonly string[]) => boolean;

Add isRequired flag option #141

Add isRequired flag option #141

Conversation

sbencoding commented Mar 24, 2020 • edited by sindresorhus Loading

What I did

Questions/suggestions

Referenced issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbencoding commented Apr 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbencoding commented May 6, 2020

sindresorhus commented May 7, 2020 • edited Loading

Add `isRequired` flag option #141

Add `isRequired` flag option #141

sbencoding commented Mar 24, 2020 •

edited by sindresorhus

Loading

sindresorhus commented May 7, 2020 •

edited

Loading