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

Swift-ify Documentation #166

Conversation

jpsphaxer
Copy link

Mostly covers TurboNavigator Documentation, with the exception of a couple files.

Made some small changes to the project's documentation with hopes to align more with Swift's DocC. Although most are esthetic changes .
You may preview these changes in XCode by Option + Click on element with doc comments.

cc: @joemasilotti - From previous PR on TurboNavigator repo

Mostly covers TurboNavigator Documentation, with the exception of a few files
@joemasilotti
Copy link
Member

Thanks for migrating your PR, @jpsphaxer! This is awesome, I love how nice it looks in Xcode now.

One question, though. It looks like all of the protocol functions in TurboNavigatorDelegate are marked as Required and have a note stating they are optional. Is that expected?

image

@jpsphaxer
Copy link
Author

e all of the protocol functions

Hey @joemasilotti, I was thinking that overriding these functions was optional since a default implementation is provided. The idea was to replace the first line in the doc comments saying something like

/// Optional. Accept or reject a visit proposal 
/// ... 

For a note saying so instead

Would you prefer that we leave this doc comment as originally intended?

@joemasilotti
Copy link
Member

You're correct, the functions are indeed optional because of the default implementations. I'm confused as to why the generated Xcode documentation says they are both required and optional.

image

@jpsphaxer
Copy link
Author

You're correct, the functions are indeed optional because of the default implementations. I'm confused as to why the generated Xcode documentation says they are both required and optional.

image

OHH i see, let me do some digging into this. Great catch btw, I didn't see it.

@jpsphaxer
Copy link
Author

@joemasilotti so the TL:DR - protocols are contracts and methods inside them are required by default. Even if we give a default implementation.

We could change the protocol to follow objc conventions and add @optional but that could limit the protocol's functionality and would require everything inside the protocol to be objc conformant.

I instead have pushed a change and propose we give a general doc note at the top level of the TurboNavigatorDelegate protocol explaining the optional implementation of the methods instead of each method's doc individually.

@joemasilotti
Copy link
Member

Thanks for this! Merging in now.

@joemasilotti joemasilotti merged commit 4813e5e into hotwired:turbo-navigator Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants