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

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

Sources/ArgumentParser/Parsable Properties/Option.swift

  public init(
    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

Sources/ArgumentParser/Parsable Properties/Option.swift

+  ///   - 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

Sources/ArgumentParser/Parsable Properties/Option.swift

  public init(
    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

Sources/ArgumentParser/Parsable Properties/Option.swift

+    completion: CompletionKind,
+    help: ArgumentHelp


Suggested change

completion: CompletionKind,

help: ArgumentHelp

completion: CompletionKind?,

help: ArgumentHelp?

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

Sources/ArgumentParser/Parsable Properties/Option.swift

@@ -138,12 +138,41 @@ extension Option where Value: ExpressibleByArgument {
  ///   - 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

McNight commented Jan 17, 2022 •

edited

Loading

rauhul Jan 17, 2022

McNight Jan 18, 2022

natecook1000 Jan 19, 2022

rauhul Jan 17, 2022

McNight Jan 18, 2022

McNight Jan 18, 2022 •

edited

Loading

rauhul Jan 18, 2022

natecook1000 left a comment

natecook1000 Jan 19, 2022

natecook1000 Jan 19, 2022

natecook1000 Jan 19, 2022

McNight Jan 19, 2022

McNight commented Jan 19, 2022

rauhul commented Jan 20, 2022

natecook1000 commented Jan 21, 2022

McNight commented Jan 21, 2022

natecook1000 left a comment

natecook1000 commented Jan 25, 2022

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

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

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

Conversation

McNight commented Jan 17, 2022 • edited Loading

Checklist

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

McNight Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natecook1000 left a 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

McNight commented Jan 19, 2022

rauhul commented Jan 20, 2022

natecook1000 commented Jan 21, 2022

McNight commented Jan 21, 2022

natecook1000 left a comment

Choose a reason for hiding this comment

natecook1000 commented Jan 25, 2022

McNight commented Jan 17, 2022 •

edited

Loading

McNight Jan 18, 2022 •

edited

Loading