Skip to content

Allow required array arguments, options, and flags #196

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

Merged
merged 4 commits into from
Aug 14, 2020
Merged

Allow required array arguments, options, and flags #196

merged 4 commits into from
Aug 14, 2020

Conversation

MPLew-is
Copy link
Contributor

@MPLew-is MPLew-is commented Jun 25, 2020

Un-deprecates (but changes the semantics of) the initializers for an array value type without a default, forcing the user to specify at least one value from the command line.

The motivation behind this is to allow users of this framework to force a user to provide at least one argument, without having to provide custom error handling. An example of a command that uses this argument setup is cp, which has a variant for one or more source_file arguments:
cp [-R [-H | -L | -P]] [-fi | -n] [-apvX] source_file ... target_directory

This unfortunately does un-do the deprecations done In #193 and also is a source-breaking change in a similar vein as the one in #170 (changing the semantics of the case where no default is provided) and so will need to wait for the next minor release as per the documentation. I'm happy to sit on this and occasionally re-integrate master into it until you're ready for a new release, but figured I'd at least get it in for review.

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

@MPLew-is
Copy link
Contributor Author

Also, does this need a documentation update? It's a purely additive change (when excluding deprecations) from what's on master, so it won't affect any existing documentation; however, I'm not sure if, how, or where you wanted to point this feature out, either.

@natecook1000
Copy link
Member

@MPLew-is Thanks for this PR! This is a great addition, as zero-or-more and one-or-more are the primary expectations for array properties. Agreed that this will need to wait until 0.3.0.

For documentation, I think we'll want to add a section to guide 2 about how defaults work in general; this can be an example there.

@natecook1000
Copy link
Member

@MPLew-is Ready to take this change — want to deal with the merge conflicts?

@MPLew-is
Copy link
Contributor Author

Sure! I’ll push a resolution for them tonight, so you should be able to merge tomorrow morning.

Un-deprecates (but changes the semantics of) an initializer for an array value type without a default, forcing the user to specify at least one value from the command line.
Extends the parent commit to arguments, still un-deprecating and changing the semantics of the previous initializer to force users to provide a value on the command line.
Extends the previous commits to flags, still un-deprecating and changing the semantics of the previous initializer to force users to provide a value on the command line.
@MPLew-is MPLew-is changed the title Allow required array argument and options Allow required array arguments, options, and flags Aug 14, 2020
@MPLew-is
Copy link
Contributor Author

I've resolved the merge conflicts and extended this to Flag arrays (which I apparently missed the first time around), so this should be good to merge pending any review you have. I also took a first pass at updating the documentation, but it was a quick stab so may need some updating.

@natecook1000
Copy link
Member

@swift-ci Please test

@natecook1000
Copy link
Member

Tests and the doc additions look great, thank you!

@natecook1000
Copy link
Member

@swift-ci Please test

@natecook1000 natecook1000 merged commit 8dfa177 into apple:master Aug 14, 2020
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.

2 participants