Deprecate Option initializer and add a new one with parameters in order by McNight · Pull Request #391 · apple/swift-argument-parser

McNight · 2022-01-17T21:31:29Z

This PR marks an Option initializer as deprecated because it had its last 2 parameters inverted (help should be before completion, following order of all other initializers).
It also adds a new one with parameters in correct order.
It follows recommendations of @natecook1000 in issue #381.

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

rauhul · 2022-01-17T22:44:53Z

    wrappedValue: Value,
    name: NameSpecification = .long,
    parsing parsingStrategy: SingleValueParsingStrategy = .next,
-    completion: CompletionKind? = nil,


This change isn't ABI stable if I recall correctly, but im not sure if that matters for this library. I wonder if you could leave types as Optional with a default of nil and use @_disfavoredOverload to prefer using your new initializer.

@natecook1000 to comment on if this is necessary

I didn't know about @_disfavoredOverload. Indeed, it seems to do the trick (when re-establishing optionals).

We don't need to worry about ABI stability, since this isn't shipping as a binary library, but for source stability we need the parameter types to stay the same. The default values can go, however, which should be enough to prevent ambiguity between this deprecated version and the new one.

rauhul · 2022-01-17T22:45:19Z

+  ///   - wrappedValue: A default value to use for this property, provided implicitly by the compiler during property wrapper initialization.
+  ///   - name: A specification for what names are allowed for this flag.
+  ///   - parsingStrategy: The behavior to use when looking for this option's value.
+  ///   - help: Information about how to use this option.


Could you add a description of the completion parameter?

Fixed. I noticed that a lot of completionKind parameters from other initializers are not documented. What should we do about those ?

Probably worth doing in a separate pull request (if you're up to it)

natecook1000

Thanks so much for taking care of this, @McNight! 🎉

In addition to the couple of notes below, could you also add a test that uses the deprecated version of the @Option initializer? It could go in DefaultsEndToEndTests.swift.

natecook1000 · 2022-01-19T03:36:30Z

    wrappedValue: Value,
    name: NameSpecification = .long,
    parsing parsingStrategy: SingleValueParsingStrategy = .next,
-    completion: CompletionKind? = nil,


We don't need to worry about ABI stability, since this isn't shipping as a binary library, but for source stability we need the parameter types to stay the same. The default values can go, however, which should be enough to prevent ambiguity between this deprecated version and the new one.

natecook1000 · 2022-01-19T03:36:50Z

+    completion: CompletionKind,
+    help: ArgumentHelp


Suggested change

completion: CompletionKind,

help: ArgumentHelp

completion: CompletionKind?,

help: ArgumentHelp?

natecook1000 · 2022-01-19T03:46:00Z

  ///   - name: A specification for what names are allowed for this flag.
  ///   - parsingStrategy: The behavior to use when looking for this option's value.
  ///   - help: Information about how to use this option.
+  @available(*, deprecated, message: "Use init(wrappedValue:name:parsing:help:completion:) instead.")


Let's be really specific here, since only people who specify both help and completion will see this message. Does this read well to you?

Suggested change

@available(*, deprecated, message: "Use init(wrappedValue:name:parsing:help:completion:) instead.")

@available(*, deprecated, message: "Swap the order of your 'help' and 'completion' arguments.")

Also, I usually remove all but the first line of documentation when deprecating something, just to make it appear less valuable.

That's great (I sometimes lack of inspiration when writing documentation or deprecation messages 😅)

McNight · 2022-01-19T22:42:43Z

I added a test. Please feel free to criticize it. Also:

I took inspiration from the testParsing_OptionPropertyInit* tests
what do you think of the spot where I put it ?
what do you think of the naming ?
it obviously generates a warning. is it an issue ?

rauhul · 2022-01-20T00:59:51Z

I think you can silence the warning by making the test deprecated and moving the command definition into the test function.

natecook1000 · 2022-01-21T15:33:06Z

Yep! If you mark the struct and test as deprecated it should eliminate the warnings. Otherwise this looks ready! 👍🏻

McNight · 2022-01-21T18:55:11Z

Both work fine; I just marked the struct and test as deprecated because I think it reads better, if that's ok for you @rauhul :)
(Next PR will be one to harmonize documentation regarding completionKind in initializers)

natecook1000

Looks great! 🎉

natecook1000 · 2022-01-25T05:31:51Z

@swift-ci Please test

rauhul reviewed Jan 17, 2022

View reviewed changes

McNight force-pushed the mcnight/deprecates_option_inverted_parameters branch from dc75893 to e518d1f Compare January 18, 2022 00:31

natecook1000 reviewed Jan 19, 2022

View reviewed changes

McNight force-pushed the mcnight/deprecates_option_inverted_parameters branch from e518d1f to 11f65d8 Compare January 19, 2022 22:38

McNight requested review from natecook1000 and rauhul January 19, 2022 22:42

Deprecate Option initializer and add a new one with parameters in order

870285f

McNight force-pushed the mcnight/deprecates_option_inverted_parameters branch from 11f65d8 to 870285f Compare January 21, 2022 18:51

natecook1000 reviewed Jan 25, 2022

View reviewed changes

natecook1000 merged commit 2b352c0 into apple:main Jan 25, 2022

natecook1000 mentioned this pull request Feb 14, 2022

Unable to set default value of String-typed option with help and completion #381

Closed

2 tasks

	@available(*, deprecated, message: "Use init(wrappedValue:name:parsing:help:completion:) instead.")
	@available(*, deprecated, message: "Swap the order of your 'help' and 'completion' arguments.")

Conversation

McNight commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

McNight Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natecook1000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

McNight commented Jan 19, 2022

Uh oh!

rauhul commented Jan 20, 2022

Uh oh!

natecook1000 commented Jan 21, 2022

Uh oh!

McNight commented Jan 21, 2022

Uh oh!

natecook1000 left a comment

Choose a reason for hiding this comment

Uh oh!

natecook1000 commented Jan 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

McNight commented Jan 17, 2022 •

edited

Loading

McNight Jan 18, 2022 •

edited

Loading