Skip to content

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Jan 10, 2023

Bug/issue #, if applicable:

Summary

This PR updates the @DirectiveArgumentWrapped property wrapper with more initializer to support more combinations of arguments. It also marks some initializers as explicitly unavailable so that DocC developers gets specific error messages if they create an unsupported configurations.

This PR also extracts the code that creates the "type display name" for directive arguments so that the same naming code applies to basic arguments, optional arguments, and completely custom arguments. This is accompanied by a handful of tests.


These changes are easies to talk about using the added tests as examples.

Support for optional values without an explicit nil default value.

Without these changes, directives with an optional value can't omit = nil, regardless if they're a built in type or a custom type. In other words, these 3 properties fail to compile without these changes

@DirectiveArgumentWrapped
var optionalBoolean: Bool?

@DirectiveArgumentWrapped
var optionalNumber: Int?

@DirectiveArgumentWrapped
var optionalCustomValue: Something?

Display of default values for optional types

Without these changes, directives with default values displayed differently depending on if the type was an optional or not. For example, these two directive arguments

@DirectiveArgumentWrapped
var booleanWithDefault: Bool = false

@DirectiveArgumentWrapped
var optionalBooleanWithDefault: Bool? = true

would display Bool = false as the type information for booleanWithDefault but Bool? as the type information for optionalBooleanWithDefault.

With these changes both those directives display their type information as Bool = false.

From the author facing perspective, the directive syntax doesn't have a syntax to explicitly spell out nil so an optional value with a non-optional default is effectively a non-optional value (either the author specifies an explicit non-nil value or the the argument uses the default non-nil value).

Requiredness of optional value with allowed values

Without these changes, an optional value with an explicit collection of "allowed values" was incorrectly marked as "required". For example

@DirectiveArgumentWrapped(
    parseArgument: { _, _ in nil },
    allowedValues: ["one", "two", "three"])
var optionalBooleanWithAllowedValues: Bool?

With these changes the implicit nil value is taken into consideration, meaning that the directive argument is marked a not required.

Dependencies

n/a

Testing

Manual tests can look at the added unit tests for inspiration of some directive arguments configurations to try specifying.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary

Also, generate more consistent `typeDisplayName` values
@d-ronnqvist d-ronnqvist force-pushed the test-directive-argument-wrapper branch from 7f13c4e to c0d2e25 Compare January 10, 2023 18:37
@d-ronnqvist d-ronnqvist marked this pull request as ready for review August 8, 2023 22:55
},
{
"text" : "or the card image used to represent this page when using the ``Links`` directive and the ``Links\/detailedGrid``"
"text" : "or the card image used to represent this page when using the ``Links`` directive and the ``Links\/VisualStyle\/detailedGrid``"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These documentation changes was done before this PR but we hadn't updated the DocC SGF to reflect the latest documentation changes.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 633dfa2 into swiftlang:main Feb 1, 2024
@d-ronnqvist d-ronnqvist deleted the test-directive-argument-wrapper branch February 1, 2024 14:11
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