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

Update option directive naming #448

Merged
merged 7 commits into from
Jan 23, 2023

Conversation

d-ronnqvist
Copy link
Contributor

Bug/issue #, if applicable:

Summary

This updates the current option directives to follow the naming convention that's proposed in this forum post.

Dependencies

n/a

Testing

Use the AutomaticArticleSubheading, AutomaticSeeAlso, and AutomaticTitleHeading option directives in content.

  • The syntax for disabling the default behaviors should remain the same as before.
  • The syntax for redundantly enabling these features now use an explicit "enabled" value.

Checklist

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

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

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@DirectiveArgumentWrapped(name: .unnamed)
public private(set) var behavior: Behavior
public private(set) var enabledness: Enabledness
Copy link
Contributor

Choose a reason for hiding this comment

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

I can definitely be convinced that Enabledness is the best word here – I'm mostly worried about it from a documentation perspective for folks less familiar with English... Just to offer an alternative – what do you think of state: State? It's in a state of enabled or disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also couldn't think a good word here. What I really want it to describe is a Bool value that can be created with "enabled"/"disabled" instead of "true"/"false" so that the directive can be written as @Directive(disabled) instead of @Directive(enabled: false).

After thinking about this some more I decided to add a new initializer to the property wrapper that allows for a Bool value with custom true/false spellings. That way we don't need to define a custom type for it however we also don't get to document the meaning of the values but it should be clear enough with a var enabled: Bool property what the two values are.

Updated in e450d41

Copy link
Contributor

Choose a reason for hiding this comment

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

Would we expect the declaration in documentation for this to be:

@Directive(_ enabled: Bool)

then?

I'm worried that will be more confusing since other instances of Bool in declarations accept true false as arguments. The documentation for the allowed values will help but it will make the declaration potentially less clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would it help if this also supported true and false as arguments?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so... I'm just more concerned about the user-facing Directive documentation than the API framework documentation. For the Directive itself – using something like Enabledness or State is probably better than Bool since it indicates that this is a different thing than: https://ethan-kusters.github.io/swift-docc/documentation/docc/choice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what the right internal API is for this. I think that the code should expose a Bool value—but that could be done with a computed property—but the user-facing Directive documentation should show something more specific than Bool.

For the type name to surface in the directive documentation I prefer Enabledness over State. To me "state" implies the computed runtime value (which may change) as opposed to a user configuration. Also, "state" is unnecessarily broad and could have more states than enabled/disabled. Even if it's the first time the developer sees the word "enabledness" it should be fairly easy to discard what it is and what it's possible values are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll revert back to an Enabledness enum but will surface a computed Bool value to the internal API.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds good to me. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I've updated this to use an "enabledness" enum again and have regenerated the directives symbol graph file.

Screenshot 2023-01-17 at 4 11 37 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great! Thank you.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

# Conflicts:
#	Sources/docc/DocCDocumentation.docc/DocC.symbols.json
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit f85fe77 into apple:main Jan 23, 2023
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.

None yet

2 participants