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

Support multiple values for an option #36

Closed
hofan41 opened this issue Jun 12, 2015 · 14 comments · Fixed by #41
Closed

Support multiple values for an option #36

hofan41 opened this issue Jun 12, 2015 · 14 comments · Fixed by #41
Assignees
Labels
Milestone

Comments

@hofan41
Copy link
Contributor

@hofan41 hofan41 commented Jun 12, 2015

Hi,
I would like to begin working on hapijs/lab#285 and in order to support this feature, it is necessary for bossy to somehow parse multiple values in for a given option. I am not quite sure what the appropriate pattern for this might be, either a comma separated list:

--reporter lcov,clover,html

Or whether they should be specified multiple times but added to an array

--reporter lcov --reporter clover --reporter html

I would definitely also like to try implementing this feature in bossy myself, so any guidance on enabling this behavior (or if there is another pattern you would like to suggest) I am all ears.

Thank you!

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Jun 13, 2015

I vote for the 2nd approach, it is more unixy.

@sericaia

This comment has been minimized.

Copy link
Contributor

@sericaia sericaia commented Jun 14, 2015

I also prefer the second! :)
Em 13/06/2015 11:10, "Nicolas Morel" notifications@github.com escreveu:

I vote for the 2nd approach, it is more unixy.


Reply to this email directly or view it on GitHub
#36 (comment).

@hofan41

This comment has been minimized.

Copy link
Contributor Author

@hofan41 hofan41 commented Jun 15, 2015

It seems like this functionality is already enabled via line 126-131:

            if (flags[name]) {
                flags[name] = [].concat(flags[name], value);
            }
            else {
                flags[name] = value;
            }

This currently only supports non-falsey values, so I will fix this "bug" and submit a PR.

@hofan41

This comment has been minimized.

Copy link
Contributor Author

@hofan41 hofan41 commented Jun 15, 2015

Another interesting dilemma, it seems number, range, and string types should allow for multiple values, with boolean, it seems like that should be disallowed. Agree?

Edit: Nevermind, realized boolean values should just be assumed true if the flag is provided.

@hofan41

This comment has been minimized.

Copy link
Contributor Author

@hofan41 hofan41 commented Jun 15, 2015

Another dilemma that presents itself is that sometimes the resultant value could be an array, and sometimes it could be a single value.

Should we actually introduce an explicit option specifying whether a value can contain multiple values or not? And if it is multiple, it will always be an array. If it is not a multiple, then it will always be a single value.

@sericaia sericaia closed this in #37 Jun 16, 2015
@sericaia sericaia reopened this Jun 16, 2015
@sericaia

This comment has been minimized.

Copy link
Contributor

@sericaia sericaia commented Jun 16, 2015

Sorry, I've closed this with the PR. I think I prefer not to have an explicit option.

@hofan41

This comment has been minimized.

Copy link
Contributor Author

@hofan41 hofan41 commented Jun 16, 2015

It would become rather verbose to check for the presence of a value when there are 3 things a value could be: null, array, single value

if ( example.reporter == 'lcov' || example.reporter.indexOf('lcov') != -1 ) 

The benefit of the explicit option would be to reduce logic like this from appearing whenever a flag can have multiple values.

@sericaia

This comment has been minimized.

Copy link
Contributor

@sericaia sericaia commented Jun 26, 2015

Hello @hofan41,

If you plan to do something like the following, just go ahead.

--vodoo --multiple xptoVar1 --vodoo --multiple xptoVar2
@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Jun 26, 2015

This doesn't look very understandable. What about adding a flag (multiple ?) to the options of switches ? It would default to false to keep from breaking changes, if it is true the value will always be null or an array, if it is false business as usual, or latest declared value, I don't know what's the current behavior.

@hueniverse hueniverse added the request label Jun 28, 2015
@hueniverse hueniverse added this to the 1.0.4 milestone Jun 28, 2015
@hofan41

This comment has been minimized.

Copy link
Contributor Author

@hofan41 hofan41 commented Jun 29, 2015

@Marsup your comment regarding multiple being a switch option in the api is what I was originally suggesting. Sorry if I confused anybody regarding my ask for a multiple option, it is via the api configuration and not from the cli.

I disagree with the suggestion that if the multiple option is false, then it defaults to original behavior. This is because I would also like the falseness of multiple to imply the value will always be the value specified in type, and not an array (except if the value is range). If a switch between explicit/original behavior is what you actually want, maybe we can also add a strict option.

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Jun 30, 2015

I don't understand what you mean, when false, it'll never be an array, it'll do whatever the library currently does.

@hofan41

This comment has been minimized.

Copy link
Contributor Author

@hofan41 hofan41 commented Jun 30, 2015

@Marsup bossy has always unofficially supported multiple values even before this issue was created. Therefore, if we allow the library to do what it already does, values can be an array, single value, or null.

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Jun 30, 2015

Oh sorry then, very weird default behavior, maybe it's worth a breaking change ?

@hofan41

This comment has been minimized.

Copy link
Contributor Author

@hofan41 hofan41 commented Jun 30, 2015

I think it is worth it. In that case this issue might need to be moved to a new milestone. My latest PR implements the multiple option as the breaking version, where true implies array/null, and false implies type/null.

@sericaia sericaia closed this in #41 Jul 1, 2015
@sericaia sericaia modified the milestones: 2.0.0, 1.0.4 Jul 5, 2015
@Marsup Marsup added feature and removed request labels Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.