Add `isMultiple` option for flags #143

ulken · 2020-04-11T21:32:17Z

Differs in two ways from previous attempts:

Instead of setting the last value if a flag is used multiple times without multiple set, it throws an error. Makes more sense to me and is how I interpret "we should allow only one value by default".
It supports boolean and number arrays in addition to string.

Fixes #111
Closes #123

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

#111: Add setting to indicate a flag can be set multiple times

index.js

- Break out into separate PR

ulken · 2020-04-15T12:15:39Z

Extracted unrelated change into separate PR: #145

sindresorhus · 2020-04-23T08:17:24Z

index.d.ts

@@ -24,14 +25,16 @@ declare namespace meow {
 		- `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.
+		- `multiple`: Indicates a flag can be set multiple times. Returns an array. (Default: false)


What returns an array? It's not clear enough.

Valid point. Maybe drop the word return altogether?

How about Values are turned into an array (in order?)?

Went with the shorter form

sindresorhus · 2020-04-23T08:19:02Z

index.js

@@ -71,6 +84,13 @@ const meow = (helpText, options) => {
 		};
 	}

+	if (minimistOptions.array !== undefined) {
+		minimistOptions.array = arrify(minimistOptions.array).map(flagKey => ({


Can you add a short code comments what's happening here?

Better yet: Poke @vadimdemedes to give his input on supporting this in minimist-options: minimist-options | Support boolean and number arrays. Then there wouldn't be any need for a comment (or code!) at all...

Your input would be highly valuable as well!

index.js

.gitignore

sindresorhus · 2020-04-23T08:21:46Z

The main function is getting huge. Would be great if you could try to modularize some of the new code into separate functions.

sindresorhus · 2020-04-23T08:24:27Z

Should it be named isMultiple instead of multiple? Since there will soon be a isRequired option (#131).

ulken · 2020-04-23T19:54:09Z

The main function is getting huge. Would be great if you could try to modularize some of the new code into separate functions.

Sure, I'll give it a go.

ulken · 2020-04-23T19:57:24Z

Should it be named isMultiple instead of multiple? Since there will soon be a isRequired option (#131).

Personally, I tend to use is/has/should-prefix when it's a function, but not for simple boolean values.

Having that said, I definitely think naming of booleans should follow the same convention. So if isRequired is set in stone, I think this should follow suit.

Also, I think you referred to the wrong issue/PR (think you meant #141). FYI: In the issue you linked to, you are referring to the issue itself instead of the PR (131 vs. 132).

ulken · 2020-04-23T21:30:28Z

@sindresorhus Alright, I've processed your feedback. Let me know what you think.

ulken · 2020-04-27T14:48:01Z

Up-to-date with master and reverted unintentional formatting change

test.js

ulken · 2020-05-05T15:49:55Z

@sindresorhus Up-to-date with master again...

sindresorhus · 2020-05-05T16:17:13Z

Looks great 👌

ulken · 2020-05-05T16:21:25Z

Cool. Would you be able to ask @vadimdemedes to check out the PR in minimist-options if you talk to him? Won’t respond on GitHub nor Twitter.

Understand he might not want to merge it, just want it to be evaluated.

sindresorhus · 2020-05-05T16:25:58Z

Give it a little bit more time. He's probably busy. Sometimes it takes some weeks for me to look at PRs too.

ulken · 2020-05-05T16:29:04Z

Hehe, yeah, so I’ve learned 😉

I get that, but he hasn’t responded to a related issue from August (I think) last year.

Oh, well. Thanks anyway.

sindresorhus · 2020-05-10T07:38:59Z

I tried this on a project and it crashes in:

/Users/sindresorhus/dev/oss/capture-website-cli/node_modules/meow/index.js:86␊
    		[flags[flagKey].type || 'string']: true␊
    		                ^␊
    ␊
    TypeError: Cannot read property 'type' of undefined␊
        at arrify.map.flagKey (/Users/sindresorhus/dev/oss/capture-website-cli/node_modules/meow/index.js:86:19)␊
        at Array.map (<anonymous>)␊
        at convertToTypedArrayOption (/Users/sindresorhus/dev/oss/capture-website-cli/node_modules/meow/index.js:84:22)␊
        at meow (/Users/sindresorhus/dev/oss/capture-website-cli/node_modules/meow/index.js:148:25)␊
        at Object.<anonymous> (/Users/sindresorhus/dev/oss/capture-website-cli/cli.js:9:13)␊
        at Module._compile (internal/modules/cjs/loader.js:778:30)␊
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)␊
        at Module.load (internal/modules/cjs/loader.js:653:32)␊
        at tryModuleLoad (internal/modules/cjs/loader.js:593:12)␊
        at Function.Module._load (internal/modules/cjs/loader.js:585:3)␊

When the flag is --hide-elements. I think it doesn't correctly account for camel-casing.

ulken · 2020-05-10T17:04:02Z

Ouch. OK, I’ll have a looksie later tonight.

ulken · 2020-05-10T20:54:28Z

@sindresorhus Pushed fix

vadimdemedes · 2020-05-17T19:13:41Z

Excuse me for delay, guys. Change is in master and published!

ulken · 2020-05-17T19:15:49Z

629af48 😉

ulken added 4 commits April 11, 2020 01:03

🙈 Ignore .vscode directory

Verified

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

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

d8bcbcc

🏷️ TypeScript type for multiple option

15e8bd2

📝 Document multiple option

8ada7b6

❇️ Add multiple setting to flags

c1f2641

ulken commented Apr 11, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

🚨 Add missing semicolon

56785a4

This was referenced Apr 11, 2020

Add setting to indicate a flag can be set multiple times #111

Closed

Add test for #9 (and deal with #16) sindresorhus/electron-better-ipc#31

Merged

⏪ Revert variable rename

6ebe454

- Break out into separate PR

ulken mentioned this pull request Apr 15, 2020

Code style tweak #145

Merged

ulken added 2 commits April 16, 2020 21:46

Merge branch 'master' into flag-multiple

384f5ce

Fix after merge

097cb44

sindresorhus requested changes Apr 23, 2020

View reviewed changes

ulken added 6 commits April 23, 2020 22:06

Remove .vscode from .gitignore

b6a843b

Update error message

36389d2

Add comment as to why we patch minimist-options output

7bbc7c1

Attempt to clarify docs

42e8366

Modularize main function

a4d4a36

multiple -> isMultiple

5602f41

ulken added 2 commits April 27, 2020 16:44

Merge branch 'master' into flag-multiple

873cfdf

Revert formatting change

5fb9b1a

ulken requested a review from sindresorhus April 27, 2020 14:47

Update index.js

88b76f0

Update test.js

f2a3ad1

sindresorhus reviewed May 5, 2020

View reviewed changes

test.js Show resolved Hide resolved

ulken added 2 commits May 5, 2020 17:44

Add multiple default values test

ed116eb

Merge branch 'master' into flag-multiple

1060bcc

sindresorhus changed the title ~~Add setting to indicate a flag can be set multiple times~~ Add isMultiple option for flags May 5, 2020

sindresorhus merged commit c4c5ee2 into sindresorhus:master May 5, 2020

ulken deleted the flag-multiple branch May 10, 2020 20:22

ulken mentioned this pull request May 10, 2020

Fix isMultiple not handling multi-word flags #150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Add `isMultiple` option for flags #143

Add `isMultiple` option for flags #143

ulken commented Apr 11, 2020 •

edited by issuehunt-oss bot

Loading

ulken commented Apr 15, 2020

sindresorhus Apr 23, 2020

ulken Apr 23, 2020

ulken Apr 23, 2020

sindresorhus Apr 23, 2020

ulken Apr 23, 2020

ulken Apr 23, 2020

sindresorhus commented Apr 23, 2020

sindresorhus commented Apr 23, 2020

ulken commented Apr 23, 2020

ulken commented Apr 23, 2020 •

edited

Loading

ulken commented Apr 23, 2020 •

edited

Loading

ulken commented Apr 27, 2020

ulken commented May 5, 2020

sindresorhus commented May 5, 2020

ulken commented May 5, 2020

sindresorhus commented May 5, 2020

ulken commented May 5, 2020

sindresorhus commented May 10, 2020

ulken commented May 10, 2020

ulken commented May 10, 2020

vadimdemedes commented May 17, 2020

ulken commented May 17, 2020

Add isMultiple option for flags #143

Add isMultiple option for flags #143

Conversation

ulken commented Apr 11, 2020 • edited by issuehunt-oss bot Loading

Referenced issues

ulken commented Apr 15, 2020

sindresorhus Apr 23, 2020

Choose a reason for hiding this comment

ulken Apr 23, 2020

Choose a reason for hiding this comment

ulken Apr 23, 2020

Choose a reason for hiding this comment

sindresorhus Apr 23, 2020

Choose a reason for hiding this comment

ulken Apr 23, 2020

Choose a reason for hiding this comment

ulken Apr 23, 2020

Choose a reason for hiding this comment

sindresorhus commented Apr 23, 2020

sindresorhus commented Apr 23, 2020

ulken commented Apr 23, 2020

ulken commented Apr 23, 2020 • edited Loading

ulken commented Apr 23, 2020 • edited Loading

ulken commented Apr 27, 2020

ulken commented May 5, 2020

sindresorhus commented May 5, 2020

ulken commented May 5, 2020

sindresorhus commented May 5, 2020

ulken commented May 5, 2020

sindresorhus commented May 10, 2020

ulken commented May 10, 2020

ulken commented May 10, 2020

vadimdemedes commented May 17, 2020

ulken commented May 17, 2020

Add `isMultiple` option for flags #143

Add `isMultiple` option for flags #143

ulken commented Apr 11, 2020 •

edited by issuehunt-oss bot

Loading

ulken commented Apr 23, 2020 •

edited

Loading

ulken commented Apr 23, 2020 •

edited

Loading